Problem/Motivation
In #3205480: Drop PECL YAML library support in favor of only Symfony YAML, the yaml_parser_class setting was "deprecated" but in fact it was totally removed. This means that sites utilizing this setting break prior to 11.0, which seems to be counter to the idea of deprecations with breaking changes only in major versions.
I imagine that only a small subset of sites utilized this setting, but there's also no compelling reason to not do a proper deprecation dance. In my case, I utilize a custom Yaml loader to allow for interpreting PHP constants. This is being worked on for core at #2951046: Allow parsing and writing PHP class constants and enums in YAML files, but until that goes in, there's no option to customize this logic on a site-specific basis. I would also argue that allowing for an overridden Yaml loader class should not be deprecated at all, as there may be future cases where site owners want to customize this behavior beyond what Drupal provides. With this setting removed, there is no longer a hook point to do this aside from a local patch to core.
Steps to reproduce
Run a site with a yaml_parser_class setting specified in Drupal 10 >= 10.3 and the setting is not respected. As this was only deprecated, I would expect a deprecation warning and the site to run as it had previously.
Proposed resolution
Add in a BC layer for this deprecation. Potentially revert the deprecation overall, for reasons explained above.
Remaining tasks
User interface changes
Introduced terminology
API changes
Data model changes
Release notes snippet
| Comment | File | Size | Author |
|---|---|---|---|
| #50 | 3485296-nr-bot.txt | 91 bytes | needs-review-queue-bot |
| #29 | 3485296-more-info.patch | 640 bytes | alexpott |
| #24 | 2024-11-12-D10_3_8-core-yaml-fix-snip.patch | 741 bytes | emptyvoid |
Issue fork drupal-3485296
Show commands
Start within a Git clone of the project using the version control instructions.
Or, if you do not have SSH keys set up on git.drupalcode.org:
- 3485296-minimal-fix
changes, plain diff MR !10189
- 3485296-regression-deprecation-of
changes, plain diff MR !10048
Comments
Comment #2
bradjones1Comment #3
longwaveFor the reasoning behind forcing the Symfony YAML parser see #3108309-36: Support Yaml::PARSE_CUSTOM_TAGS in \Drupal\Component\Serialization\YamlSymfony::decode which let us implement e.g. #3414208: Add support for tagged_iterator to YamlFileLoader in 10.3.x. PECL YAML does not allow arbitrary custom tags at parse time, they must be specified up front.
Comment #4
bradjones1I appreciate the pointers to these issues re: the Symfony Yaml parser, however I don't see how this is pertinent to the issue of not being able to provide your own overridden config. This isn't a function of the PECL parser.
Are you saying that a "proper" deprecation dance isn't possible here?
Comment #5
longwaveThe problem is if someone was using the PECL parser then it would error out when building the container if those new tags are used. Reverting this now is non trivial unfortunately.
Are you using a custom extension of the Symfony parser? I guess we could enforce that the parent class is the Symfony base class, but allow the class to be overridden? Are there any actual implementations outside of Symfony and PECL?
Comment #6
bradjones1If you're using the PECL parser AND the extensions aren't supported, that's on you IMO.
I am only extending the Symfony parser to allow for functionality that is forthcoming in #2951046: Allow parsing and writing PHP class constants and enums in YAML files but I'd rather just keep my predictable code path and upgrade with 11.x than add another patch to core on my project. I think there's a case to be made also to maintain some sort of hook point for a custom parser for the reason that core might not always have the specific parser config (e.g., flags) you want.
It seems to me that this was removed, not deprecated, and this is a BC break in violation of our usual way of doing things. Seems to me that a deprecation should mean the existing functionality works in 10.x as much as possible. At the very least the deprecation should be re-cast as a breaking-change removal of functionality and not a deprecation, which it isn't right now.
Comment #8
longwaveWondering if we can do something like MR!10048? Perhaps this can stay permanently, I don't see that it hurts anything.
Comment #9
smustgrave commentedJust moving to NW as that MR seems to break all the tests.
Comment #10
alexpottI think this is okay to re-introduce. I'm not convinced this should be functionality we should support forever but breaking it in 10.3 was indeed a break so we could re-introduce in the way that @longwave is suggesting. OTOH @bradjones1 could maintain and apply the patch in #2951046: Allow parsing and writing PHP class constants and enums in YAML files using composer patches. It'd be great to land that.
Comment #11
catchI had never thought of overriding the yaml parser to provide actual functionality as opposed to just switching between symfony and pecl, looks harmless to bring it back like this.
Comment #12
bradjones1OK, so the MR is updated for 10.5.x, it keeps the deprecation but allows this to honor BC.
I went back and forth on whether to propose "keeping" this functionality in D11, as some have noted here and elsewhere there are legitimate use cases for swapping in your own Yaml parser. I am only doing so to get functionality that will come in #2951046: Allow parsing and writing PHP class constants and enums in YAML files... BUT this shouldn't have broken D10 sites to begin with and we need to fix that.
Comment #13
alexpottThis looks good to go and should be merged back to 10.3.x
Comment #17
longwaveCommitted and pushed 4a8aaa4a9ca to 10.5.x and ebba8e798f6 to 10.4.x and eb382d27768 to 10.3.x. Thanks!
There will be an out of band patch release in 10.3 this week to fix critical issues, so might as well squeeze this one in too.
Comment #19
emptyvoid commentedYeah um.
Just updated the site core to 10.3.8 and BOOM.
Even commenting out the yaml settings all of my builds now white screen with errors via drush.
How do I fix this?
I'll have to downgrade all my projects at this point to keep it stable.
Comment #20
longwaveWhat is
yaml_parser_classset to in settings.php?Comment #21
emptyvoid commentedThis properly validates if the class even exists and loads correctly.
Even with this patch, disabling the yaml settings in both settings.php and or settings.local.php borks all of the 10.3.8 sites I manage.
White screens, apache errors.. Drush failure.. the works!
Keeping the settings with this patch allows normal operation.
Comment #22
emptyvoid commentedHere is the settings prior to the update:
settings.php
and local settings for my dev instance:
settings.local.php
If I comment out one or all the caching error appears.
Comment #23
sidgrafix commented@emptyvoid thank you patched from #21 worked
Had same issue WSOD with pretty much identical stack trace.
Comment #24
emptyvoid commentedOpps sorry, the previous patch caught all of the core patches I maintain..
Rerolled with just the yaml snippet patch.
Comment #25
longwaveAre you sure there is nothing setting
yaml_parser_class? What is$classset to when this happens?The only way I can reproduce this is by setting
yaml_parser_classto an entirely invalid value, such as TRUE or a non-empty array.If I set it to an invalid class name then I get a useful error:
I don't think we should fail silently if the setting is incorrect.
Comment #26
sidgrafix commented@longwave I tried completely removing line:
# $settings['yaml_parser_class'] = NULL;from settings.php and it still WSOD, only until applying patch now from #24 do any of my sites work with 10.3.8, otherwise I have to pin to 10.3.7 or use the patchI'm also a little confused? If # comments the option out, I would assume that meant it was inactive (as I have never changed or used this setting) I think it's finding the class from somewhere else as of 10.3.8 since 10.3.7? I also note that in the latest default has a message regarding it being deprecated in 10.3 and removed in 11. So if I wasn't using and also removed it after updating to 10.3.8 and still get the WSOD for the class. Shouldn't whatever the change was (from 10.3.7 to 10.3.8) account for that? I see this being a big problem for a lot of installs out there. If not what should that be set to to prevent a WSOD; yaml_parser_class = ?
The patch in #24 is the only thing that worked for me.
I also ran into this on 1 of 2 sites: (also shown above in comment #21)
You have requested a non-existent service "cache.backend.null". Did you mean one of these: "cache.backend.memory", "cache.backend.apcu", "cache.backend.php- Which was fixed after removing a $settings[] in the settings.php
- should anyone wonder where they originate; likely any cache setting configured that used cache.backend.null service (which I'm guessing that service was removed in 10.3.7 as I had that issue when I initial pinned to 10.3.7 when initially getting WSOD's) not 100% sure on that, just appears that way.
Comment #27
catchComment #28
longwaveI'm unable to reproduce this on a fresh install without deliberately setting
yaml_parser_classas per #25, so steps to reproduce this would be really helpful.Comment #29
alexpottIf people could apply the patch and clear caches and restart the webserver this would let us know what is in yaml_parser_class setting - there must be something otherwise it would not be failing.
Comment #30
emptyvoid commentedThis may be anecdotal, but I've noticed that only sites running on Acquia Cloud appears affected for me.
I'd have to do a full source code compare to determine how a vanilla source tree and one built on Acquia Cloud differ.
But source trees built and running on other platforms like Pantheon or a vanilla AWS stack seem fine.
A full source tree search on vanilla 10.3.8 shows:
Comment #31
longwaveOn both 10.2 and 10.3.8 I can still only reproduce this by setting
yaml_parser_classto something that isn't a string. In both cases if I set it to a string but for a class that doesn't exist I get a different "Class not found" error.@sidgrafix are you on Acquia Cloud too? I am still curious what
Settings::get('yaml_parser_class')returns on this platform, and why it has started failing here but doesn't on 10.2.Comment #32
sidgrafix commented@longwave No none of my installs are on Acquia Cloud.
Comment #33
alexpott@sidgrafix, @emptyvoid: Can someone please run
vendor/bin/drush ev "var_dump(\Drupal\Core\Site\Settings::get('yaml_parser_class'));"from the command line. We need to see what you have in that setting in order to understand why this is breaking.Comment #34
sidgrafix commented@alexpott running
drush ev "var_dump(\Drupal\Core\Site\Settings::get('yaml_parser_class'));"returns:bool(true)Comment #35
bradjones1Thought - could it be that at least in some cases this value was set to a PECL parser, which would be incompatible with #3205480: Drop PECL YAML library support in favor of only Symfony YAML? We would perhaps need a test/conditional to validate the provided class is a Symfony parser? That is the part of the BC layer here that we really can't provide without backing out the whole of the change that got us here, in the first place.
Comment #36
sidgrafix commented@alexpott and @bradjones1
With the patch from #29 I now get an error when navigating admin pages.
Not sure if that helps any..
Comment #37
alexpott@bradjones1 the Pecl yaml parser still works - I tested this already.
@sidgrafix where is yaml_parser_class set? It should never be a boolean - that would not have worked on 10.2.x - see #31.
Comment #38
sidgrafix commented@alexpott I don't have it set anywhere! I've never set it, other than seeing it available in settings.php where else would it be possible to set it?
Comment #39
catchIf Acquia is injecting settings into settings.php (via environment variables, an include etc.), it could be set there.
Comment #40
sidgrafix commented@catch I don't use Acquia and just checked all custom theme and module files for any hooks that may be setting yaml_parser_class and cannot find anything injecting it. I also don't have any env's or includes and nothing in php ini configs.
Is it possible the setting was a default set by core on install back in Drupal 9.x somewhere and it wasn't updated/changed properly on existing installs during an update.
Comment #41
alexpott@sidgrafix can you do
grep -R yaml_parser_class ./on the Drupal root directory of the server where you are experiencing the problem.Comment #42
sidgrafix commented@alexpott Appears to show up only in default expected locations and inactive!
grep -R yaml_parser_class ./returns:Comment #43
alexpott@sidgrafix well how does it get set to TRUE... there must be something somewhere setting it to TRUE. Is your settings.php including files from outside Drupal root?
Comment #44
sidgrafix commented@alexpott, nope it isn't set anywhere and I can honestly say I never set it for any reason on either of the installs with this issue. Both of which had been in development since Drupal 9.x
I'll try to dig into things deeper by looking back thru every Drupal release prior to see when the setting was original introduced into Drupal and see if I can figure out how a setting I've never used and likely others was set as a Boolean when clearly it should not be. My best guess is somewhere in Drupal release history the setting was used as a Boolean then changed and not properly removed or changed on installation of an update.
Comment #45
longwave@sidgrafix These settings are not stored in the database - they are only configured in code through settings.php. And this setting has never been a boolean, since it was introduced in #1920902: Add a Drupal Yaml wrapper so we can default to PECL Yaml component if it is available it's always been a string.
Comment #46
emptyvoid commented@alexpot
On my local dev the config setting is:
NULLOn Acquia Cloud it returns:
bool(true)Comment #48
alexpottPushed a very simple fix for people where this has been set to TRUE for whatever reason. Very odd and no idea how it worked on 10.2 but as this feature is removed in 11.x I think we can just be pragmatic and forget about this.
Comment #49
dmitry.korhov@alexpott
Drupal version : 10.3.5
PHP version : 8.3.13
Drush version : 12.5.3.0
Comment #50
needs-review-queue-bot commentedThe Needs Review Queue Bot tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".
This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.
Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.
Comment #51
nod_probably a false positive
Comment #52
nod_Comment #53
longwaveGiven the only cases in the wild that we know of are where it is somehow set to TRUE I think this is the pragmatic solution for now, this only needs to remain until Drupal 10 EOL so we have no maintenance concerns going forwards.
Would still love to know exactly how it is being set to TRUE in these environments!
Comment #57
catchCommitted/pushed to 10.5.x and cherry-picked to 10.4.x and 10.3.x, thanks!
Comment #59
sidgrafix commented@catch, I'm a little confused! Was a fix not implemented and committed (#54) for Drupal 10.3.x?
I just updated to 10.3.9 and removed the patch from #24 and the issue still persists with WSOD! Had to put patch back in. Patch #24 is the only one that works without issues as well. If I use the patch for more info from #29 there is no WSOD however I get the same error repeated like 70+ times filling pages of logs with
User warning: Class "1" does not exist.if Idrush cr or drush updb.Comment #60
longwave@sidgrafix this week's releases only contained security fixes. Bug fix releases are scheduled for the first Wednesday of the month; this fix will be released in the first week of December.