Skip to content

Use RecordDataFormatter.ini instead of hard coded function mapping #4333

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 8 commits into from
Apr 14, 2025

Conversation

rominail
Copy link
Contributor

Prevent use of hard coded mapping in RecordDataFormatterFactory.php
Benefits, when someone wants to customize their instance and add for example toc[] = Summary to their displayed fields (our case) no need to override any function but just add the corresponding function mapping.

@demiankatz demiankatz requested a review from ThoWagen April 10, 2025 19:43
Copy link
Member

@demiankatz demiankatz left a comment

Choose a reason for hiding this comment

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

Thanks, @rominail -- the only thing I would change is to default the array to the existing hard-coded version if no section is defined in RecordDataFormatter.ini, for better backward-compatibility with existing configuration files. Otherwise, things might break in unexpected ways if people fail to upgrade their configuration files.

We should also check whether the VuFind\Config\Upgrade class is set up to manage RecordDataFormatter.ini; I imagine it's possible that it's not doing anything with that file yet, and this addition would justify adding support. (If you're not familiar with that class, I can help with that part of the work if you need me to!).

I'm also interested in hearing @ThoWagen's thoughts on this, since he's been doing a lot of work related to RecordDataFormatter recently.

@rominail
Copy link
Contributor Author

I made the modification for the hard coded array however I'm not familiar with the upgrade class

Copy link
Contributor

@ThoWagen ThoWagen left a comment

Choose a reason for hiding this comment

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

Thanks @rominail, I believe this is helpful!

However, I have some comments below about the location of the logic and the defaults.

@demiankatz
Copy link
Member

I made the modification for the hard coded array however I'm not familiar with the upgrade class

I'll wait until you've had a time to address @ThoWagen's review, and then I can help ensure that the configuration upgrade works right with whatever design we settle on.

@rominail
Copy link
Contributor Author

@ThoWagen I made the changes, I was not sure about the implementation of the default though
Don't hesitate if it's not what you expected
I was thinking to set defaults in both, to be able to remove the hard coded value in DefaultRecord in the future

…ltsFunctionMapping

# Conflicts:
#	module/VuFind/src/VuFind/View/Helper/Root/RecordDataFormatterFactory.php
Copy link
Contributor

@ThoWagen ThoWagen left a comment

Choose a reason for hiding this comment

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

Currently, the DefaultRecord only has those four methods so if you want to change the used methods you would have to override the class anyway to add new methods and therefore I'm not sure if it would make sense to replace the hard coded values with the config. But maybe I am missing something here.

Could you please elaborate on why you want to make this change?

@rominail
Copy link
Contributor Author

I think it makes less code, remove redundancy and on a personal touch I don't like hard coded values
But I don't mind leaving it like this

Copy link
Contributor

@ThoWagen ThoWagen left a comment

Choose a reason for hiding this comment

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

Thanks @rominail, this is looking good to me.

@demiankatz
Copy link
Member

Thanks, @rominail and @ThoWagen! I'll see if I can find time to work on the configuration upgrade on Monday; if I fail to get to it then, this may have to wait a little longer, since I will be away at a conference for most of next week. Apologies in advance if I cause any delays. :-)

@ThoWagen
Copy link
Contributor

@demiankatz I believe there is no need for the configuration upgrade anymore :)

@demiankatz
Copy link
Member

@demiankatz I believe there is no need for the configuration upgrade anymore :)

Even though no special logic is needed, basic handling for RecordDataFormatter.ini needed to be added to ensure that new sections/comments are carried forward. I've taken care of that here.

It would probably be a good idea to make the config upgrade tool smarter, so that it automatically upgrades new files without requiring explicit methods... but that's a larger project for another day. :-)

@demiankatz demiankatz added this to the 11.0 milestone Apr 14, 2025
@demiankatz demiankatz merged commit fff300d into vufind-org:dev Apr 14, 2025
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants