Issue #3150153: Bind paragraph view mode with form mode introduced some issues involving the hooks. Currently the implementation is like this:
function paragraph_view_mode_entity_view_mode_alter(string &$view_mode, EntityInterface $entity): void
But the actual hook it's implementing from core is setup like this:
function hook_entity_view_mode_alter(&$view_mode, \Drupal\Core\Entity\EntityInterface $entity)
The key difference, being that the $view_mode parameter is being strongly typed to a string in the new implementation. The Drupal documentation does indicate that they want a string, but they're not actually enforcing that right now. Apparently not all modules follow that soft rule though so it's causing PHP errors on a few of my sites:
TypeError: paragraph_view_mode_entity_view_mode_alter(): Argument #1 ($view_mode) must be of type string, null given
In this case it seemed to be related to rendering a block content entity using bamboo twig, but I've seen it crop up some other ways as well. One could make the argument that the other modules should update their implementations, but I think a change like that should be driven by a change to the core hook, not a community module.
Removing the strongly typed parameter and adding a check in the function to be sure it has a string populated in $view_mode before passing it to the matchViewForModeAndEntity fixes the wsod and retains the same flow for when there is a view mode to look at.
I did the same thing for the form mode hook, for the same reasons.
| Comment | File | Size | Author |
|---|---|---|---|
| paragraph_view_mode-less-strict-hook-implementation.patch | 2.12 KB | jacobbell84 |
Comments
Comment #3
sayco commentedThank you for reporting indeed seems like I was too strict (actually I wasn't aware that Core does not have strict typing for this argument).
I also agree that the relevant changes should be done first in Drupal core, then we could adapt it to a more strict mode.
I've merged your patch partially. I've reworked a bit of it related to the if statement - use the early return instead of wrapping the whole block with if.
Comment #4
jacobbell84 commentedGreat, thank you!
Comment #5
scotwith1tI'm confused...the patch isn't implemented, but rather a different approach which doesn't seem to address the underlying concern, namely that the typing in the definition is stricter than the hook it is implementing. I am applying the patch instead of just using the latest version and that fixes my issue. I don't think the approach in 3.1-dev (not 3.1.1) goes far enough and am re-opening for another look.
It also might make sense to add a check of the entity type while you're at it, something like
if ($entity->getEntityTypeId() !== 'paragraph') return;to ensure even further that this hook implementation doesn't over-reach. Thanks!
Comment #6
jacobbell84 commentedHi scotwith1t,
That's a good idea on the entity type check. Concerning what was committed though, while different then my original patch, does still address the root cause of the issue as far as I can tell. https://git.drupalcode.org/project/paragraph_view_mode/-/commit/e7ccca92...
Removing the 'string' parameter type was the main part of the fix, which I see there and on the current dev release.
Comment #7
sayco commented@scotwith1t Thank you for sharing your thoughts on this issue. It's as @jacobbell84 said. The main difference between the original patch and the new approach is that the latter uses an early return statement instead of executing the code in the if statement. However, both versions should function the same way and address the underlying concern.
Based on that fact I assume that the original problem has been resolved? Is this correct? Or is it for some reason working differently for you on the dev branch?
If you feel that there's a specific problem that's different from the original issue (which in my opinion was solved), it would be best to create a new issue and provide detailed information there. This will help to ensure that the issue is properly tracked and addressed.
BTW it is worth mentioning that I have implemented a proper checking, but for the ParagraphInterface instead of the entity id. Please have look on DisplayModeMatcher https://git.drupalcode.org/project/paragraph_view_mode/-/blob/3.x/src/Matcher/DisplayModeMatcher.php#L80 for more details.
Comment #8
scotwith1tThanks folks. All that makes sense. Moving forward with the latest dev and looking forward to a new stable release with that in the future. Thx!
Comment #9
sayco commented