joegraduate → made their first commit to this issue’s fork.
@tadean also noted that the removal of the ProxyClass and changing the lazy behavior of the config.installer service probably constitutes a performance regression (probably especially for sites using Drupal core < 11.2?).
It does seem like it would be desirable to keep the lazy behavior / ProxyClass if possible but the changes to the ConfigInstallerInterface introduced in 11.2 (the reason for this issue) make that a much more complicated task (because 2 different versions of the ProxyClass are needed to provide compatibility with Drupal >=11.2 and Drupal <11.2.
Discussed the 2 different MR approaches extensively with fellow maintainer @tadean.
I've updated MR !20 with the approach that we both feel the most comfortable with which registers our ConfigProviderConfigInstaller class as a decorator for the core config.installer service via the services.yml file instead of altering (replacing) the core service via a ServiceProvider class. This approach vastly simplifies the MR because it:
- removes the need for the existing ServiceProvider and Proxy classes,
- removes the need for the compatibility traits previously added in MR !20
- requires no changes to the existing
ConfigProviderConfigInstallerservice class
I've also hidden the other MR branch.
While discussing this issue and the potential fixes with @tadean, we realized that the changes proposed in @trackleft2's MRs to address 📌 Address conflict with Features over config.installer class Active . We might want to consider closing this issue as a duplicate of that one and updating its IS with the 11.2 incompatibility details.
joegraduate → changed the visibility of the branch 3532693-dynamic-alter to hidden.
Tested MR !20 with an existing project that contains a custom ConfigProvider plugin and found that the decorator approach used in that MR doesn't seem to work (the custom ConfigProvider plugin I was testing did not work as expected).
Created alternate MR !21 that adds a separate backwards-compatible `ConfigProviderConfigInstaller` class and updates the existing service provider to alter the core service using the appropriate `ConfigProviderConfigInstaller` class depending on the installed core version. The custom ConfigProvider in our existing project did still work as expected when tested with this MR.
Updated module to decorate core config.installer service rather than alter it in MR !20. This made it easier to use the dynamic compatibility traits strategy employed by the config_selector module and made it possible to remove the proxy class. The proxy class was needed before because this module was extending the core ConfigInstaller class but that is no longer the case in MR.
joegraduate → created an issue.
Re-rolled @ericgsmith's patch from #38 into a new MR targeting 11.x.
Also attaching MR diff as a static patch that should be usable with Drupal 11.2.x.
Leaving status as "Needs work" since I'm not sure whether reverting the validation changes added in 10.2.x is still being considered as an option for resolving this issue or not.
joegraduate → made their first commit to this issue’s fork.
Created a new branch and MR from the existing MR targeting 11.x. Also applied some PHPStan fixes and the config schema fix described in #136.
Attaching diff of new MR as a static patch for composer that should be usable with 11.2.x and 10.5.x.
@smustgrave, I guess I'm not sure how to best go about implementing a deprecation since the current functionality is built into the user module (in a hook_ENTITY_TYPE_insert() implementation that gets triggered any time a user role entity is created) and not part of any API code that would be invoked directly by contrib extensions.
We could add something to trigger a deprecation warning inside the hook implementation but there wouldn't really be anything contrib extension maintainers could do to address the deprecation until functionality contained in the hook implementation is removed or moved elsewhere.
I'm also not sure whether this constitutes a major change or not. @alexpott mentioned in #18 🐛 user_user_role_insert should not exist Needs work that what user_user_role_insert() currently does isn't strictly needed:
user_user_role_insert() is doing something that does not have to happen at all. It's a UI niceness but from an API point-of-view just gets in the way.
- Addressed @smustgrave's MR comments
- Added the actions as exported config yml files to config/install in the standard and demo_umami install profiles
- Added the actions as exported config yml files in the administrator_role and content_editor_role recipes
@smustgrave, yes, I think if contrib module tests involve the creation of user roles, they could be affected by these changes. I think maintainers of profiles and recipes that include custom roles are even more likely to be affected.
Added a post_update to the existing MR.
joegraduate → made their first commit to this issue’s fork.
Created a new 11.x MR (and hid the original 11.x MR branch). Assuming that the the NR bot only checks the most recently created MR for compatibility with 11.x.
joegraduate → changed the visibility of the branch 2672340-remove-user_user_role_insert to hidden.
Hid 10.5.x MR.
joegraduate → changed the visibility of the branch 2672340-remove-user_user_role_insert-10.5.x to hidden.
joegraduate → changed the visibility of the branch 11.x to hidden.
joegraduate → changed the visibility of the branch 10.5.x to hidden.
Created MR targeting 10.5.x.
joegraduate → changed the visibility of the branch 2672340-remove-user_user_role_insert-10.4.x to hidden.
Updated the MR with @alexpott's recommended changes and also replaced strpos() with preg_match() using a regex that should prevent the false positives due to substring overlap that @alexpott noted by ensuring that the matches are either at the beginning of the query string or after a &.
joegraduate → made their first commit to this issue’s fork.
Created a draft change record: https://www.drupal.org/node/3522105 →
Also made a second MR targeting 10.4.x
Re-rolled patch from #26 into a MR targeting 11.x.
joegraduate → made their first commit to this issue’s fork.
Good to see you on d.o. again @nedjo!
Just wanted to call out contrib project that the folks at Centarro created that might be useful here (possibly as an alternative to relying on Project Browser?):
https://www.drupal.org/project/recipe_tracker →
LGTM. +1 for RTBC.
LGTM. Seems like this would be fine to merge after 🐛 Address cspell warnings found in gitlab job Active .
LGTM
No longer applies to 11.x / 11.0.x / 11.1.x as of 📌 [11.x] Require array argument for AddCssCommand Fixed .
Addressing this issue will probably also solve a related issue: ✨ SyncFilterDeriver uses getExtensionChangelists for getDerivativeDefinitions Active (or possibly make it obsolete).
joegraduate → changed the visibility of the branch 3.0.x to hidden.
Merged.
Merged.
LGTM
joegraduate → made their first commit to this issue’s fork.
joegraduate → made their first commit to this issue’s fork.
Merged. Leaving open for project update bot.
Should be good to go now.
LGTM
Merged.
No longer needed now that we merged 📌 Automated Drupal 11 compatibility fixes for config_distro Needs review .
Merged into 2.1.x. Leaving issue open for project update bot.
joegraduate → made their first commit to this issue’s fork.
Merged. Thanks all!
Merged.
joegraduate → made their first commit to this issue’s fork.
Merged. Thanks all!
Merged.
LGTM. Thanks @trackleft2.
Added some review comments/suggestions to MR.
Created a separate follow-up issue to fix this: 🐛 Breaking API change in #3457459 broke config_sync Active .
joegraduate → created an issue.
Adding latest MR diff as static patch file for composer.
joegraduate → made their first commit to this issue’s fork.
@skrug Could you create a new issue with as many details as possible?
Hi @damienmckenna. We had a site that was repeatedly throwing this error (on every web request) until we applied this patch and cleared cache, but unfortunately we still don't understand exactly why and haven't identified steps to reproduce the error. We're suspicious that some bad/incomplete render data was cached on the site and that was causing the error but we're not totally sure about that.
We aren't using any other patches on the site but the Schema Metatag module and several of its submodules had recently been installed on the site before we saw this error.
Here is the full stack trace from one of the instances of this error on our site:
Backtrace
#0 /code/web/modules/contrib/metatag/src/MetatagManager.php(284): uasort(NULL, Array)
#1 /code/web/modules/contrib/metatag/src/MetatagManager.php(609): Drupal\metatag\MetatagManager->sortedTags()
#2 /code/web/modules/contrib/metatag/src/MetatagManager.php(565): Drupal\metatag\MetatagManager->generateRawElements(Array, Object(Drupal\node\Entity\Node))
#3 /code/web/modules/contrib/metatag/metatag.module(508): Drupal\metatag\MetatagManager->generateElements(Array, Object(Drupal\node\Entity\Node))
#4 /code/web/modules/contrib/metatag/metatag.module(262): metatag_get_tags_from_route()
#5 /code/web/modules/contrib/metatag/metatag.module(222): _metatag_remove_duplicate_entity_tags(Array)
#6 /code/web/core/lib/Drupal/Core/Extension/ModuleHandler.php(552): metatag_entity_view_alter(Array, Object(Drupal\node\Entity\Node), Object(Drupal\Core\Entity\Entity\EntityViewDisplay))
#7 /code/web/core/lib/Drupal/Core/Entity/EntityViewBuilder.php(305): Drupal\Core\Extension\ModuleHandler->alter('node_view', Array, Object(Drupal\node\Entity\Node), Object(Drupal\Core\Entity\Entity\EntityViewDisplay))
#8 /code/web/core/lib/Drupal/Core/Entity/EntityViewBuilder.php(239): Drupal\Core\Entity\EntityViewBuilder->buildMultiple(Array)
#9 [internal function]: Drupal\Core\Entity\EntityViewBuilder->build(Array)
#10 /code/web/core/lib/Drupal/Core/Security/DoTrustedCallbackTrait.php(113): call_user_func_array(Array, Array)
#11 /code/web/core/lib/Drupal/Core/Render/Renderer.php(870): Drupal\Core\Render\Renderer->doTrustedCallback(Array, Array, 'Render #pre_ren...', 'exception', 'Drupal\\Core\\Ren...')
#12 /code/web/core/lib/Drupal/Core/Render/Renderer.php(432): Drupal\Core\Render\Renderer->doCallback('#pre_render', Array, Array)
#13 /code/web/core/lib/Drupal/Core/Render/Renderer.php(248): Drupal\Core\Render\Renderer->doRender(Array, false)
#14 /code/web/core/lib/Drupal/Core/Render/MainContent/HtmlRenderer.php(238): Drupal\Core\Render\Renderer->render(Array, false)
#15 /code/web/core/lib/Drupal/Core/Render/Renderer.php(638): Drupal\Core\Render\MainContent\HtmlRenderer->Drupal\Core\Render\MainContent\{closure}()
#16 /code/web/core/lib/Drupal/Core/Render/MainContent/HtmlRenderer.php(231): Drupal\Core\Render\Renderer->executeInRenderContext(Object(Drupal\Core\Render\RenderContext), Object(Closure))
#17 /code/web/core/lib/Drupal/Core/Render/MainContent/HtmlRenderer.php(128): Drupal\Core\Render\MainContent\HtmlRenderer->prepare(Array, Object(Symfony\Component\HttpFoundation\Request), Object(Drupal\Core\Routing\CurrentRouteMatch))
#18 /code/web/core/lib/Drupal/Core/EventSubscriber/MainContentViewSubscriber.php(90): Drupal\Core\Render\MainContent\HtmlRenderer->renderResponse(Array, Object(Symfony\Component\HttpFoundation\Request), Object(Drupal\Core\Routing\CurrentRouteMatch))
#19 [internal function]: Drupal\Core\EventSubscriber\MainContentViewSubscriber->onViewRenderArray(Object(Symfony\Component\HttpKernel\Event\ViewEvent), 'kernel.view', Object(Drupal\Component\EventDispatcher\ContainerAwareEventDispatcher))
#20 /code/web/core/lib/Drupal/Component/EventDispatcher/ContainerAwareEventDispatcher.php(111): call_user_func(Array, Object(Symfony\Component\HttpKernel\Event\ViewEvent), 'kernel.view', Object(Drupal\Component\EventDispatcher\ContainerAwareEventDispatcher))
#21 /code/vendor/symfony/http-kernel/HttpKernel.php(186): Drupal\Component\EventDispatcher\ContainerAwareEventDispatcher->dispatch(Object(Symfony\Component\HttpKernel\Event\ViewEvent), 'kernel.view')
#22 /code/vendor/symfony/http-kernel/HttpKernel.php(76): Symfony\Component\HttpKernel\HttpKernel->handleRaw(Object(Symfony\Component\HttpFoundation\Request), 1)
#23 /code/web/core/lib/Drupal/Core/StackMiddleware/Session.php(53): Symfony\Component\HttpKernel\HttpKernel->handle(Object(Symfony\Component\HttpFoundation\Request), 1, true)
#24 /code/web/core/lib/Drupal/Core/StackMiddleware/KernelPreHandle.php(48): Drupal\Core\StackMiddleware\Session->handle(Object(Symfony\Component\HttpFoundation\Request), 1, true)
#25 /code/web/core/lib/Drupal/Core/StackMiddleware/ContentLength.php(28): Drupal\Core\StackMiddleware\KernelPreHandle->handle(Object(Symfony\Component\HttpFoundation\Request), 1, true)
#26 /code/web/core/modules/page_cache/src/StackMiddleware/PageCache.php(191): Drupal\Core\StackMiddleware\ContentLength->handle(Object(Symfony\Component\HttpFoundation\Request), 1, true)
#27 /code/web/core/modules/page_cache/src/StackMiddleware/PageCache.php(128): Drupal\page_cache\StackMiddleware\PageCache->fetch(Object(Symfony\Component\HttpFoundation\Request), 1, true)
#28 /code/web/core/modules/page_cache/src/StackMiddleware/PageCache.php(82): Drupal\page_cache\StackMiddleware\PageCache->lookup(Object(Symfony\Component\HttpFoundation\Request), 1, true)
#29 /code/web/core/lib/Drupal/Core/StackMiddleware/ReverseProxyMiddleware.php(48): Drupal\page_cache\StackMiddleware\PageCache->handle(Object(Symfony\Component\HttpFoundation\Request), 1, true)
#30 /code/web/core/lib/Drupal/Core/StackMiddleware/NegotiationMiddleware.php(51): Drupal\Core\StackMiddleware\ReverseProxyMiddleware->handle(Object(Symfony\Component\HttpFoundation\Request), 1, true)
#31 /code/web/core/lib/Drupal/Core/StackMiddleware/AjaxPageState.php(36): Drupal\Core\StackMiddleware\NegotiationMiddleware->handle(Object(Symfony\Component\HttpFoundation\Request), 1, true)
#32 /code/web/core/lib/Drupal/Core/StackMiddleware/StackedHttpKernel.php(51): Drupal\Core\StackMiddleware\AjaxPageState->handle(Object(Symfony\Component\HttpFoundation\Request), 1, true)
#33 /code/web/core/lib/Drupal/Core/DrupalKernel.php(741): Drupal\Core\StackMiddleware\StackedHttpKernel->handle(Object(Symfony\Component\HttpFoundation\Request), 1, true)
#34 /code/web/index.php(19): Drupal\Core\DrupalKernel->handle(Object(Symfony\Component\HttpFoundation\Request))
#35 {main}
Created a MR based on proposed resolution in IS. Attaching as a patch usable for composer as well.
joegraduate → made their first commit to this issue’s fork.
Increasing priority to major since it is impossible to save module config currently.
This sounds like a duplicate of 🐛 Can't save image widget crop settings in Drupal 11 Active
Manually triggered the "tests only" pipeline on the MR and saw that the new test added passes even without the other MR changes which I don't think is the expected result.
Spoke to @trackleft2 about this on Slack and he mentioned that he thinks he needs to add something to the test that verifies the generated file is still actually a multiframe image.
Merged. Thanks all!
Blocked by this 📌 Automated Drupal 11 compatibility fixes for masquerade_log Needs review
joegraduate → created an issue.
MR is ready for review.
Merged.
joegraduate → created an issue.
Merged.