Skip to content

Remove unused folder_id field from search table#2699

Merged
demiankatz merged 10 commits into
vufind-org:devfrom
demiankatz:remove-search-folder-id
Mar 14, 2023
Merged

Remove unused folder_id field from search table#2699
demiankatz merged 10 commits into
vufind-org:devfrom
demiankatz:remove-search-folder-id

Conversation

@demiankatz
Copy link
Copy Markdown
Member

@demiankatz demiankatz commented Feb 8, 2023

The search table in the database has contained a folder_id field since the early days of VuFind 1.x; however, this field has never been used for anything. This PR removes the unused field and associated code/indexes.

TODO

  • Add upgrade script logic to optionally remove field/index from MySQL (allow it to be kept in case somebody has found a local use)
  • Add migration to remove field/index from PostgreSQL
  • Run full test suite
  • Update changelog
  • Make appropriate updates to Convert from Laminas\Db to Doctrine #2233 after merging.

@demiankatz demiankatz requested a review from EreMaijala March 3, 2023 20:11
@demiankatz
Copy link
Copy Markdown
Member Author

@EreMaijala, I think this is more or less ready to go. The screen for selecting whether or not to remove the deprecated column in the upgrade process could probably be beautified, but at least it's functional. Maybe @crhallberg has some ideas there.

@demiankatz demiankatz requested a review from crhallberg March 3, 2023 20:12
Copy link
Copy Markdown
Contributor

@EreMaijala EreMaijala left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me and works as well!

@demiankatz
Copy link
Copy Markdown
Member Author

Thanks, @EreMaijala! Here's a screen shot of the new upgrade screen in case @crhallberg wants to suggest any changes to confirmdeprecatedcolumns.phtml before I merge this PR:

image

@demiankatz demiankatz added this to the 9.0 milestone Mar 6, 2023

<p><b>RECOMMENDED:</b> You can <a href="<?=$this->url('upgrade-confirmdeprecatedcolumns')?>?action=delete">remove the columns</a>.</p>

<p>Alternatively, if you are using these columns for some reason (e.g. in local custom code), you can choose to <a href="<?=$this->url('upgrade-confirmdeprecatedcolumns')?>?action=keep">keep the columns</a>.</p>
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As regular link, it looks like it will take you to documentation on how to do this. I would instead style these links as buttons by adding the classes btn btn-default.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, @crhallberg. I had to use btn btn-default btn-link to avoid having underlines inside the buttons. Is that appropriate, or should I do things differently? (If I leave off btn-default they don't look like buttons).

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That makes sense, I'm surprised that .btn-default doesn't remove the underline. That might be a more consistent option.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same applies to #2741.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please take a look at my latest changes to #2741. If you approve of that approach, I can update this to match once it is merged.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, @crhallberg, #2741 has been merged into this, and I've adjusted the template accordingly. I think this is ready to merge now; just needs your approval.

@demiankatz demiankatz requested a review from crhallberg March 13, 2023 20:50
@demiankatz demiankatz merged commit 44bee69 into vufind-org:dev Mar 14, 2023
@demiankatz demiankatz deleted the remove-search-folder-id branch March 14, 2023 19:59
EreMaijala pushed a commit to EreMaijala/vufind that referenced this pull request Oct 20, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants