Metatag is missing a dependency - drupal:serialization when used with webprofiler
| Comment | File | Size | Author |
|---|---|---|---|
| #65 | 2899234-65-metatag-normalizer.patch | 2.59 KB | shaundychko |
| #64 | 2899234-64.patch | 1.98 KB | heddn |
| #64 | interdiff_56-64.txt | 2.3 KB | heddn |
Metatag is missing a dependency - drupal:serialization when used with webprofiler
| Comment | File | Size | Author |
|---|---|---|---|
| #65 | 2899234-65-metatag-normalizer.patch | 2.59 KB | shaundychko |
| #64 | 2899234-64.patch | 1.98 KB | heddn |
| #64 | interdiff_56-64.txt | 2.3 KB | heddn |
Comments
Comment #2
hanoiiI also looked into this while looking at the related issue:
I tried to look into this. It's reproducible. The error comes from the webprofiler module when its attempting to do the reflection of the class. If the serialization module is enabled, that error doesn't happen. I tried jsonapi out of curiosity as I would have expected them to have the same issue, but they have serialization as a dependency, thus preventing this.
I am not sure what's the fix for this, if any, but likely beyond this module unless adding serialization as a dependency is viable, but I think not.
There could potentially be a fix to be done on the profiler to prevent a fatalerror on this or to reflect all classes, not only the ones of the enabled modules. But it's a bit beyond me or what I can look into this right now.
Comment #3
hanoiiWell, maybe adding serialization as a dependency is not THAT bad. Another option would be to create a metatag_serialization submodule with the dependency most of the code added by the patch on the parent issue.
Comment #4
damienmckennaThat's weird, the tests run ok so it isn't a general problem when WebProfier is enabled. I wonder if the tests don't show the problem because it's using the "testing" profile which doesn't have a full site to test against?
Comment #5
hanoiiBut what tests? The webprofiler tests? I don't think this use case is covered by tests. Metatag+Devel+Webprofiler module without serialization, or is it?
Comment #6
damienmckennaIt's like there's a problem with WebProfiler that it's loading classes it shouldn't be trying to load, breaking the DI workflow..
Incidentally, changing the test's profile to "standard" causes the tests to never finish. So there's a problem there somewhere X-)
Comment #7
damienmckennaCheck out Drupal\Tests\metatag\Functional\EnsureDevelWebProfilerWorks.
Maybe because it doesn't create any entities?
Comment #8
hanoiiAh, I didn't know you were already testing for the module.
I do feel that there's something odd on the webprofiler module.
FWIW I tried a manual install with no entities and the error is still there.
Comment #9
damienmckennaTry updating Devel to the latest -dev snapshot, does that make a difference?
Comment #10
damienmckennaThis expands the tests to create a content type and node, and then load a node.
Comment #11
damienmckennaRefactored NodeJsonOutput() to use this too.
Comment #13
damienmckennaThose tests work locally so I've committed them.
Comment #14
hanoiiDevel-1.x does the have the same issue.
Comment #15
hanoiiAnd the patch I guess doesn't really fix or add a proper test to this issue.
EDIT: As it was passing before anyway.
Comment #16
damienmckennaThe patch was so that we could have an entity existing while WebProfiler was installed, rather than just a custom route, so that WebProfiler would try to do its entity shuffling stuff.
What other modules do you have installed?
Comment #17
hanoiiMe, I tried it with a bare install, nothing other than metatag and webprofiler.
Comment #18
damienmckennaOut of interest are you using 8.3.x or 8.4.x?
Comment #19
damienmckennaI've been working on fixing the tests for 8.4.x (#2898805: Fix tests for 8.4.x) and have discovered that EnsureDevelWebProfilerWorks does indeed still fail on 8.4.x on drupalci, though it does work locally. Gah.
Comment #20
hanoii8.3.x currently
Comment #21
damienmckennaWhat version of PHP?
Comment #22
hanoiiThe bare install I tried this is on PHP 7.0.20.
I haven't tried it, but I think this error should happen with any non-core plugin if the module is not enable or even there.
Like say I have a module that defines a webform plugin to be used if webform is available but I don't even have webform, the same should happen.
Comment #23
hanoiiAlso, out of curiosity as I haven't worked so much with tests.. do you have anything special or recommendation to go about it, or just a normal durpal install? I once tried to setup the database on a ram drive or something on D7 to speed things up, but not sure if that's still necessary/worth doing.
Anyway, if you have any advise to share to get speed up getting into testing, appreciated!
Thanks!
Comment #24
damienmckennaComment #25
damienmckennaCan you please confirm if this is still a problem? Devel 1.0 was released and should fix this problem.
Comment #26
edurenye commentedWell, it does not. Sadly, the error still happens.
Comment #27
edurenye commentedAdding the missing dependency fixes the problem.
Comment #28
paladyn commented@DamienMcKenna
Problem still exists.
I am using Drupal 8.3.7, MetaTag 8.x-1.2, Devel 8.x-1.0 (with WebProfiler enabled).
Error:
Error: Class 'Drupal\serialization\Normalizer\NormalizerBase' not found in .../metatag/src/Normalizer/FieldItemNormalizer.php, line 10
I`ve added drupal:serialization dependency to metatag.info.yml.
Comment #29
edurenye commentedYou must reinstall the module or install serialization manually for this to take effect, probably we should add and update path.
But I'm not sure if adding a module with an update path could break any current site.
Comment #30
damienmckennaAdding a dependency is the wrong approach to take and isn't going to happen. The problem is with Devel or WebProfiler, it shouldn't be loading classes unnecessarily.
Comment #31
norman.lolThanks #29! Enabling Drupal's Serialization module and re-installing Metatag fixed the issue for me.
Comment #32
vvs commentedI'm from #2913860: Error after enable module config_translation. Site cruched and drush not working… How to reanimate it?
Comment #33
musa.thomasAgree with #30
Pending new patch if you can't use drush / ui to disable enble module you can pro tempore delete all content in all src/Normalizer of metatag, then enable your module, patch metatag, then put back the code inside.
Comment #34
jeffm2001 commentedDumb question, but the FieldItemNormalizer class extends
Drupal\serialization\Normalizer\NormalizerBase. Does this not mean that metatag depends on serialization? Not trying to be obnoxious here, I honestly don't know the answer.Comment #35
damienmckenna@JeffM2001: due to how the DI system works, I don't believe it should try loading those classes until a module requests them. The problem is that something is requesting them when it shouldn't be.
We could try wrapping the class in a class_exists() call, but it's a bit unhygienic.
Comment #36
jeffm2001 commentedFrom what I can tell — and I admit I am a bit out of my depth here — webprofiler is attempting to gather info on every service defined in the container (see
Drupal\webprofiler\Compiler\ServicePassclass). That's obviously not a typical scenario, but also doesn't seem strictly out of bounds.Should it be up to webprofiler to determine if another module's service has an undeclared dependency? (again an honest question)
Perhaps these services should be defined in a submodule that has serialization as a dependency.
Comment #37
jeffm2001 commentedI looked into this a bit more, and I think this might be the best way to go. Rather than define the normalizers in metatag.services.yml, it uses a ServiceProvider class to add the definitions conditionally. This patch is based on what's in the entity_reference_revisions module.
Comment #38
damienmckenna@JeffM2001: Awesome, thank you.
Anyone have any suggestions on how to reproduce the problem using tests?
Comment #39
twang commentedWe downloaded the patch #37 and tested it. It threw warnings related to blank lines with spaces. We fixed the warnings, tested, and verified that the patch works.
Comment #40
twang commentedWe tried to fix the blank line warning issues in #39 as shown by the attached screen shot, but apparently the issue is still here.
Comment #41
jcisio commentedIn #38 the maintainer asked for test, which there is not now.
BTW it seems that the cause is webprofiler module. You need to enable that module to have this bug.
Comment #42
damienmckenna@jcisio: But that's the thing - there's already a test that enables WebProfiler and it doesn't hit this error.
Comment #43
kumkum29 commentedHello,
I have broken my site after install metatag & Webprofiler.
I get always this error in my server logs.
But I don't see how to uninstall metatag / or webprofiler without the Drupal UI. I tried several Drush commands without success.
Can you give a solution to uninstall manualy metatag module?
thanks.
I'm on D8.4
PS:
When I use a drush command, i get this error:
Comment #44
kumkum29 commentedComment #45
damienmckennaAs mentioned above, a temporary workaround is to enable the Serialization module.
Comment #46
damienmckennaCan anyone please work out the steps necessary to trigger this problem? Given that the module already has tests which confirm the error doesn't happen with a bare installation, there has to be some extra piece that causes it to fail.
Comment #47
hanoiiI don't think it doesn't happen with a bare drupal.
I just run a simpletest.me with this and devel, enabled metatag and webprofiler and I got:
Fatal error: Class 'Drupal\serialization\Normalizer\NormalizerBase' not found in /home/du1jz/www/sites/default/modules/metatag/src/Normalizer/FieldItemNormalizer.php on line 10
And it became broken: https://du1jz.ply.st/ (up for the next 23h, not that is any useful like this).
I think the test, for some reason, is not really triggering this.
Comment #48
damienmckenna@hanoii: Ok then, lets try this.
Comment #49
damienmckennaThe tests still pass and they pass locally. That said, my local D8 build *does* fail.
Comment #50
kumkum29 commented@DamienMcKenna
Unfornately, this patch don't resolved my broken site.
I haven't access to Drupal admin UI, and drush commands don't work.
Is there another method to uninstall metatag module ?
Comment #51
damienmckennaOk. Does this work? It checks to make sure that the NormalizerBase class is available.
Comment #53
damienmckennaOk, try it a different way.
Comment #54
kumkum29 commentedYes !!
my site is back !!!
Now, I can uninstall the webprofiler module.
Thanks ;)
(with patch 51)
Comment #56
jeffm2001 commented@DamienMcKenna: I still think the approach in my patch (#37) is the way to go.
The fundamental problem is defining a service that is extending a class which may or may not exist. Your patch in #53 does seem to fix the error, but it looks strange to me to have a condition just floating there outside the class. I think it's better to not have the service registered at all if the module it depends on isn't installed. This is also the approach used in entity_reference_revisions and file_entity.
As for tests, it's not clear to me why the existing test doesn't get triggered, but I don't have a ton of experience in that area. Wondering if an explicit test for webprofiler is really necessary. Obviously it would be nice, but if the patch is an improvement, solves the specific issue folks are seeing, and doesn't break any other tests, might be worth just committing.
Here's the patch again with a fix for the whitespace issue that was flagged by twang.
Comment #57
damienmckenna@JeffM2001: You are correct, especially when the conditional logic I was trying was causing the tests to fail. Your patch works great.
I'm still trying to reproduce the problem via tests.
Comment #59
damienmckennaAnd the test failure was useless:
*sigh*
So I'm planning on committing #56 but I really would prefer to be able to cause a proper test failure first.
Comment #60
damienmckennaReran the test and it still fails again. The backtrace ends with this, i.e. this is where we find the last function call before it fails:
/var/www/html/modules/contrib/metatag/tests/src/Functional/EnsureDevelWebProfilerWorks.php:42That is this line:
pullmyhairout.gif
Comment #61
jeffm2001 commentedI did some playing around with this, and from what I can tell, it might actually be impossible to trigger this error through tests. You can
assertTrue(class_exists('Drupal\serialization\Normalizer\NormalizerBase'))or any other class in the system and it will always pass. Take a look at core/tests/bootstrap.php, it appears to be finding the paths for every core/contrib extension and adding them all to the autoloader.Comment #62
heddnClosed out #2923446: webprofiler depends on serialization as duplicate.
Drive-by feedback: can this use ClassName::class syntax?
Comment #63
mile23Yes, bootstrap.php finds all existing extensions and adds them to the autoloader. This is left over from when there were only Drupal unit tests in PHPUnit, so it made sense.
This means that when you run a PHPUnit-based test you'll have autoloadable access to all the classes in serialization, which isn't so great for functional or kernel test purposes.
During run-time, this won't happen, so you could break anything that needs serialization module (or anything that isn't declared a dependency in info.yml).
Basically: Since the core module is always present in the file system, you won't be able to test the behavior of when it's unavailable.
It also seems that since
FieldItemNormalizer::normalize()saysreturn t('Metatags are normalized in the metatag field.');, #56 needs to removeFieldItemNormalizeras well. Or at least deprecate it.Comment #64
heddnHere's an attempt to respond to the feedback since #56. I've removed the needs test tag, since it doesn't seem possible. And add manual testing instead.
Comment #65
shaundychkoThe patch takes the comment in #63 that the third normalizer service needs to be moved from metatag.service.yml into the service provider class, and also edits #64 to use short array syntax.
Comment #66
dqdok, last both comments are confusing. Which one should be reviewed? Is #65 a follow up with additions or is it another approach?Oh I think I missed the last part of the last comment.Comment #67
dqdComment #68
dqdAnd I can confirm finally that the Error 500 (WSOD) disappears. So you can reach the site again. Any drawbacks or flaws aren't investigated yet. At least you are able to deactivate the respective modules now.
EDIT: no drawbacks yet with both modules still active (1 1/2 days)
Comment #69
landsman commentedHello,
I have problem on multisite site in Drush. On default site it works fine, look:
Can you say where is a problem please?
EDIT: This patch working for me now: https://www.drupal.org/files/issues/2899234-65-metatag-normalizer.patch
Comment #70
danielnolde commented2899234-65-metatag-normalizer.patch did _not_ solve the error
> Fatal error: Class 'Drupal\serialization\Normalizer\NormalizerBase' not found in /htdocs/modules/contrib/metatag/src/Normalizer/FieldItemNormalizer.php on line 10
when having metatag.module enabled with devel.module's webprofiler module.
Comment #71
jonasdk commentedAny chance this will get into the release soon?
I have tested the patch in https://www.drupal.org/project/metatag/issues/2899234#comment-12357622 and it seems to work as promissed.
I did this after applying the patch to ensure that no other problems got stuck in there.
Comment #72
mmbkI had the same issue on my development system, metag did not install while webprofiler was activated.
Applying the patch #65 solved the problem, the module is working now.
Comment #73
Georgii commented+1 for the patch #65.
In reply to #70. Did you confirm that the patch was applied? In my case I had the same story after executing git apply. And then I found that the patch just was not successfully applied until
patch -p1 < 2899234-65-metatag-normalizer.patchComment #74
Georgii commentedWith all the confirmation above may I move it forward?
Comment #76
damienmckennaCommitted. Thanks everyone!
Comment #77
damienmckennaThis was actually committed, so it can be closed.
It will be included in the next stable release, 8.x-1.5.
Comment #79
dqdThanks @all! Great work done here. Thanks @heddn & @Shaun for the final patch and thanks to @Damien for supporting, guiding and letting it in so quickly.