Make account menu configurable

[xmorave2]

The purpose of this PR is to make a menu account a bit more flexible in custom instances of VuFind.

I tried to make most of menu items features configurable, but there are some conditions which cannot be set easily through configuration, so they are still coded in the templates.

[ThoWagen]

I am currently working on something similar. I plan to add configurable links to the menu and other places that e.g. can also be links to other websites or make it possible to open a link in the lightbox.

Please let me know if you are planing something similar as a followup PR.

In any case I will wait with creating an own PR until this one is completed.

[demiankatz]

Thanks, @xmorave2 -- I haven't studied all the code changes in close detail yet; I just took a first pass to read the documentation and get a general idea of your approach. I've left some comments suggesting minor documentation improvements and a possibly helpful idea for refactoring the rest. Please let me know what you think!

[demiankatz]

One other thought: I don't really like the "MyResearch" terminology, and I'm hoping to get rid of it some day. Maybe we should use the terminology "Account Menu" in our file naming and code so we don't introduce "MyResearch" into even more places.

[xmorave2]

@ThoWagen Interetsting idea. This PR is very specific, but could be used to add some links to menu only when they would have a route in vufind, not for external ones. But could be relatively easily be modified to accept external links (and configurable ligthbox) too. If you have some use case for it, I can do it. But I was not planning to make it more general tool for adding links through the entire UI (although I agree it could be useful)

[ThoWagen]

@xmorave2 I think this is enough for the current PR. I just wanted to make sure that we don't do the same work twice :)

But I have one suggestion. Maybe it is better to use YAML instead of INI for the config. In my opinion this is the better choice for more complex configurations and that's how I planned to do it. I could also change that in a followup PR if that's wanted. But if you or @demiankatz prefer INI configs I don't mind that either.

[demiankatz]

@ThoWagen, I don't have a strong feeling about whether .ini or YAML is better for this case -- happy to go with whatever @xmorave2 prefers. I think both formats have trade-offs. As you say, YAML is better than .ini for describing complex configurations, especially if deep nesting of settings is required... but YAML is also a bit less user-friendly and more fragile than .ini in my experience (particularly when things break because of tiny whitespace issues), so I try to avoid it when it's not likely to be necessary.

[demiankatz]

@xmorave2, I'll wait on the final decision about configuration format before I do my most thorough review, but in the meantime I ran the full test suite (which passed) and made some minor adjustments to the comments in the .ini file to make them more complete based on what I saw elsewhere in the code.

One other idea for your consideration: might it be worth adding a render(string $activeOption) method on the helper, so we can replace all the:

<?=$this->context($this)->renderInContext('myresearch/menu.phtml', ['active' => 'librarycards'])?>

type calls with:

<?=$this->accountMenu()->render('librarycards')?>

? This might help raise awareness of the helper's existence, as well as make the code tidier and more consistent. If you prefer not to do this, though, I don't insist; it's just an idea!

[xmorave2]

I have moved the configuration to yaml and slightly changed its structure. I also added the render method to the helper

[demiankatz]

Thanks, @xmorave2 -- this looks really good, and I've double-checked that the refactoring looks correct and the default settings are consistent with the default configuration file.

I haven't done hands-on testing yet -- first I had a few small questions and suggestions. You are free to ignore most of these if you don't think they are helpful, but at least one or two of the suggested optimizations might be worth the effort.

Once you've made whatever final changes you feel are worthwhile, and if @ThoWagen has no further suggestions, I'll do a hands-on test and merge if all goes well.

[xmorave2]

@demiankatz Thanks for the review, I've just incorporated your suggestion, so this should be ready for another round

[ThoWagen]

I haven't done any testing, but this looks good to me and I have no further suggestions. This should be a good starting point for me to implement additional features. Thanks!

[demiankatz]

Looks good to me -- thanks, @xmorave2!