Conversation
|
In our sample BannerNotices config file we have documentation which includes a few examples of using the conditional logic. For a real world use of the |
demiankatz
left a comment
demiankatz
left a comment
There was a problem hiding this comment.
Thanks, @ThoWagen and @natecollins! I haven't fully reviewed this, but I left a few comments below based on some initial reading.
I've also created a wiki page for you: https://vufind.org/wiki/development:plugins:condition_handlers
I generally favor documentation in config files rather than the wiki so that the project is self-contained and so that version-specific documentation stays bundled with the associated code... but I see why in this case it wouldn't make sense to document the condition handlers in the Notices.yaml since they could be used in other contexts as well... so I think using the wiki is okay, as long as we clearly indicate which versions various handlers were added in.
Please let me know if you need anything else from me; I'll review more thoroughly after you've had a chance to respond to this initial review.
| * @link https://vufind.org/wiki/development Wiki | ||
| */ | ||
|
|
||
| namespace VuFind\Condition\Handlers; |
There was a problem hiding this comment.
I would change this namespace to VuFind\Condition\Handler -- we generally use singular names in namespaces.
| /** | ||
| * Get base value to check. | ||
| * | ||
| * @param array $condition Optional used for handler specific parameters |
There was a problem hiding this comment.
This says "optional" but appears to be mandatory.
| * | ||
| * @param string $comparator The type of comparison to evaluate | ||
| * @param string $baseValue The value to validate | ||
| * @param string[] $checkedValues Values for the comparison which will result in success |
There was a problem hiding this comment.
This function comment is a bit confusing; maybe better to align with the description in the array key documentation above.
| protected function getBaseValue(array $condition): string | ||
| { | ||
| $string = $condition['string'] ?? null; | ||
| if ($string === null) { |
There was a problem hiding this comment.
Is it also worth doing a type check here, in case something weird is passed through the array?
| */ | ||
| protected function getBaseValue(array $condition): string | ||
| { | ||
| return (new \DateTime())->format('Y-m-d'); |
There was a problem hiding this comment.
Should we allow a DateTime to be passed through the condition for non-default comparisons?
| * Check if a condition is met. | ||
| * | ||
| * Conditions are represented as an associative array with the following required keys: | ||
| * - type: identifier of the condition handler |
There was a problem hiding this comment.
Should we note that type is used by the condition manager to route requests but can be ignored by the actual handlers? I can't think of a situation where the handler itself would look at the type, unless you think we might have a multi-purpose handler that would get registered under multiple aliases.
3 participants
This PR adds the option to add conditions to notices.
It was mainly taken from here https://github.com/MSU-Libraries/catalog/blob/daa34afd1d1ab8ff530fb352af8deecf6a2fa9d8/vufind/module/Catalog/src/Catalog/View/Helper/Root/Notices.php#L196-L324. That is why I noted MSU in the copyright and @natecollins as author. But the condition types are now pluggable as the "condition handlers".
To keep this PR small, it only provides a handler for dates and one for static strings. The latter is used for testing and I don't know if there is actually a real world example where it would make sense to use it. @natecollins @meganschanz please let me know, if there is and I will remove the doc comment that says it is only used for tests.
More handlers will be added in a followup PR.
I want to use the conditions in upcoming PRs for other features and that's why I believe that it would be better to document the conditions in a wiki page instead of the Notices.yaml. @demiankatz will you create a page for that, so that I can insert the documentation? Not sure, if and how I can do that myself.