- Issue created by @SirClickALot
- First commit to issue fork.
- Merge request !95Issue #3465604: Fixed 'TypeError: method_exists(): Argument #1 () must be of... β (Merged) created by dydave
- Status changed to Needs review
3 months ago 8:34am 6 August 2024 - π«π·France dydave
It appears this change was introduced in commit:
https://git.drupalcode.org/project/admin_toolbar/-/commit/d549bdb3e22120...I wasn't able to reproduce the issue locally, so I'm not exactly sure how you ended up with a
null
value for$this->entity
here:
https://git.drupalcode.org/project/admin_toolbar/-/blob/3.x/admin_toolba...But could you please try the patch from MR!95 and see if it fixes the issue for you?
I tried installing admin_toolbar 3.x-dev on a fresh D11/PHP8.3 install and was unable to reproduce the error, so you perhaps have additional contrib modules or custom code that might be causing the issue.
Could you maybe try looking in your project and see what type of config entity could be causing the problem?
Maybe in the error stack trace? or try breaking in the code with a debugger (XDebug, for example)?
(This could help us reproduce the issue locally for debugging purpose)This MR is an initial first stab, but maybe it would be better to move the added null coalescing operator when
$this->entity
is initialized:
https://git.drupalcode.org/issue/admin_toolbar-3466125/-/blob/3.x/admin_...
or maybe restore some of the checks before the code was changed in:
https://git.drupalcode.org/project/admin_toolbar/-/commit/d549bdb3e22120...Any feedback, reviews and comments would be greatly appreciated.
Thanks in advance! - π¬π§United Kingdom SirClickALot Somerset
Good steer DYdave β !
You were right, it was indeed a contributing module that was causing this β Admin Toolbar Content β .
I shall raise the issue over there.
Thank you
Probably worth leaving this issue open to help others?
- π«π·France dydave
Definitely !
Thanks a lot Nick (@SirClickalot) for the positive and speedy reply, it's greatly appreciated! π
Sure! Feel free to let the users of the
admin_toolbar_content
module know and perhaps set the issues as related.Let's keep this ticket open for now, it's a good idea, as you mentioned.
At least we've got a bit more information on how the problem could be reproduced, so when I get a bit more time, I'll do a bit more testing and see if there is anything that could potentially be done in any of these modules to try to fix the problem.
Thanks again for your great help testing and reporting!
- π«π·France dydave
Based on the report of another user here:
#3466743-4: [AdminToolbarToolsSettingsForm] Fix Fatal Error: Call to undefined method Drupal\Core\Menu\MenuLinkManager::invalidateAll() βIt appears module Admin Toolbar Version β could also be affected and create the same issue when upgrading to 3.5.0.
On a local dev system with D10.2.7, PHP 8.3.9 and
admin_toolbar
admin_toolbar_links_access_filter
admin_toolbar_search
admin_toolbar_tools
admin_toolbar_contentenabled the WSOD appeared after upgrading only admin_toolbar from 3.4.2 to 3.5.0 and nothing else. Patch M!95 restored the functionality. Thanks @DYdave.
- π«π·France dydave
Thanks a lot @dgwolf!
Super helpful feedback, in particular with the detailed versions and modules π
This is definitely going to help other users searching for the same error online and getting to this issue.
Let's see if we can get more feedback on this and we'll assess whether the change should go into
admin_toolbar
or the other modules.Thanks again for taking the time to report back! π
- π§πͺBelgium kriboogh
Maintainer of admin_toolbar_content here. I can confirm that our module is causing an issue with the "recent content" menu items. The getDescription method is expecting an entity which in some case isn't the case for the recent content items. I will add a workaround for this, but I really think for good measures that in admin_toolbar, there should be a check on the $this->entity being not null for the getDescription method, since you also do it for the getTitle.
- π«π·France dydave
Hi Kris (@kriboogh), thanks a lot for getting back on this issue and the helpful comment.
Would you recommend we changed line 68:
https://git.drupalcode.org/project/admin_toolbar/-/blob/3.x/admin_toolba...
to something like this ?- if (method_exists($this->entity, 'getDescription')) { + if ($this->entity && method_exists($this->entity, 'getDescription')) {
Could you perhaps give this a quick test on a local install?
If that works for you, I'd be glad to make the corresponding changes in MR!95.Any feedback, additional comments or suggestions would be greatly appreciated.
Thanks in advance! - π§πͺBelgium kriboogh
Hi Dave (@DYDave), that should do it yes (tested that a couple of minutes ago). In the mean time I override the getDescription method in our subclass and check it also. The fix for that is in our 2.0.9 release.
But your fix to that line you mentioned would make the code more robust. - π«π·France dydave
Thanks a lot once again Kris (@kriboogh) for the positive and speedy reply, it's super helpful! π
The corresponding change has been made to MR!95, ready to be reviewed.
Just wondering if tests would be needed for this π€
Perhaps an integration test class withadmin_toolbar_content
, but that might be overkill ... talking about robustness π
What do you think? Would that be necessary?In any case, feel free to let us know if you have more advice, or encounter other errors related with the latest changes in
admin_toolbar
(post 3.5.0 update), we would greatly appreciate your feedback and reviews.
Thanks again ! - π¦πΊAustralia uqjhawk3
Can confirm after applying the MR !95 patch this resolved this issue for our codebase.
- Status changed to RTBC
3 months ago 5:10am 22 August 2024 - π³πΏNew Zealand ericgsmith
I haven't got to the bottom of why I see this error, but I only saw it in our CI testing pipeline.
Either way - the check makes the method consistent with the rest of the class and resolves the error I was seeing.
- πΊπΈUnited States iacwebdev
I ran into this exact issue today - it was triggered when I deleted an old unused user role from the system, and then immediately got a WSOD across all admin pages.
I've noticed for several years that when an entity of a type that's enumerated somewhere within the admin toolbar (e.g. user roles, content types, taxonomies, etc.) is deleted, Drupal doesn't fully delete all the details about the entity, and so it remains stuck within the admin toolbar until you get around to flushing all of the caches. It was never a big deal in the past, so I never got around to reporting the problem anywhere. I'm guessing something was changed in Admin Toolbar 2.5.0 that now tries to call a method on each of those enumerated entities, but when Drupal doesn't return a valid entity, you end up with a WSOD.
FWIW, I first tried truncating all of the cache_* tables to attempt a manual cache flush, but that didn't work. So I went into the code based on the error message details and did something akin to #14 above, but I went at it this way, to ensure that $this->entity is really an object:
if (is_object($this->entity) && method_exists($this->entity, 'getDescription')) {
That does seem like the safest thing to do to ensure that admin toolbar will never trigger a WSOD.
It would be nice for the root problem to be uncovered and fixed as well - it sounds like a number of components, in core and third-party, are enumerating entities into the main menu and not doing it right, or doing something wrong when an individual entity is deleted. The problem for me is that I'm really not sure what queue to open a ticket in regarding the deletion bug.
FWIW, I'm only running admin_toolbar, admin_toolbar_tools, and admin_toolbar_search - I don't have any other toolbar add-ons installed, so if the deletion bug is actually connected to something outside of core and connected to a toolbar/menu-related module, it would have to be one of those three.
- π³πΏNew Zealand ericgsmith
Ah that's very interesting!
It explains why I was only seeing it in CI - we had tests adding / removing content types.
Without looking through the code I suspect it is most closely related to π Allow plugin derivers to specify cache tags for their definitions Postponed: needs info - since the deriver
ExtraLinks
is creating these links it has no ability to let the plugin discovery cache know about the cache meta data of the content types - so likely the links plugin definition cache is not cleared when a content type is removed? - First commit to issue fork.
-
japerry β
committed 7cca52bc on 3.x authored by
dydave β
Issue #3465604 by dydave: TypeError: method_exists(): Argument #1 ($...
-
japerry β
committed 7cca52bc on 3.x authored by
dydave β
- Status changed to Fixed
2 months ago 8:25pm 5 September 2024 - πΊπΈUnited States japerry KVUO
Ahh sorry about that. I think the menu links was assumed there was an enttiy associated with it. Probably would like to refactor this logic more later (with a test!) but for now this is a good enough bugfix.
Automatically closed - issue fixed for 2 weeks with no activity.
- πΊπΈUnited States bkosborne New Jersey, USA
I had the same situation as #19. Got this error in my CI when running a large behat test suite, where a test deleted a user role that was created. Thanks for fixing this.
- πΊπ¦Ukraine mero.S
Patch if you don't have time to wait for module update and need something now.