Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Removes various simple BC layers in various places in Drupal\Core. More or less covering everything that doesn't have a dedicated issue yet although a few still need an issue as they seemed a bit too tricky to touch. Likely makes sense to keep this until the others are done.
Comment | File | Size | Author |
---|---|---|---|
#27 | interdiff.3104307.25-27.txt | 7.96 KB | longwave |
#27 | 3104307-27.patch | 69.56 KB | longwave |
Comments
Comment #2
BerdirComment #4
andypostDatabase changes using #3097879: Remove all @deprecated code in \Drupal\Core\Database
Comment #5
BerdirYeah, didn't saw that, this is for later anyway when the other components are done.
@todo: Remove check for stable in \Drupal\Core\Theme\ThemeInitialization::getActiveThemeByName()
Comment #6
Wim LeersCould you point to the specific issues this should be postponed on?
Comment #7
Berdirdatabase is now in, so I'd say at least #3092090: Remove legacy Path Alias subsystem and possibly #3069696: Remove BC layers from the entity system, at that point it might be easier to deal with the remaining parts in a combined patch.
Comment #8
Wim Leers#7: great, thanks! :) By sheer coincidence I just finished reviewing #3092090 (and RTBC'd it)! So hopefully we'll be down to
PP-1
soon :)Comment #9
Wim Leers#3092090: Remove legacy Path Alias subsystem landed earlier today!
Comment #10
BerdirIn regards to what is blocked on what, this is also not strictly blocked on anything, it's more a question of how much we want to do here exactly.
For example, I'm not sure how to handle \Drupal\Core\Config\ExtensionInstallStorage::__construct(), it says that all arguments are required, but the createCollection() call below only passes 3? So ignoring for now.
And we'll need a separate follow-up for \Drupal\Core\StringTranslation\TranslationWrapper, we need to redeprecate that for D10 and figure out how to get it out of places like installed entity type storage.
Also, \Drupal\Core\Messenger\LegacyMessenger might be a bit tricker/larger.
Then there is \Drupal\Core\Routing\Router::generate, not sure how to remove that? the generate() method is part of \Symfony\Component\Routing\Generator\UrlGeneratorInterface, which \Symfony\Component\Routing\RouterInterface implements, so we have to implement the method?
Then there are quite a lot of deprecation helpers like DeprecatedArray, ModuleHandler and also \Drupal\Core\Security\DoTrustedCallbackTrait::doTrustedCallback, I kept that and just changed the Renderer to throw an exception.
\Drupal\Core\Site\Settings::get() is in #2831065: Remove BC layer from \Drupal\Core\DrupalKernel::getInstallProfile()
Another interesting one is \Drupal\Core\Field\AllowedTagsXssTrait, looks like that hasn't been properly deprecated yet with @trigger_error() and is still used in a few places in core.
I also gave it a try and clean up theme stuff a bit, and for example remove base_themes and stylesheet-remove stuff, but at least stylesheet-remove was also never fully deprecated with @trigger_error(), ther's an issue somewhere.
Just trying to see where we stand with this.
Comment #11
BerdirFun, nightwatch is failing because the setup stuff is using the FunctionalTestSetupTrait without setting $defaultTheme, that triggers deprecation messages that aren't reported?
Comment #12
BerdirSplit off #3110874: Remove BC layer for TestSetupTrait as a first one.
Comment #13
longwaveIn #3110296: Remove all @deprecated code from Drupal/Core/Access/ I noticed this was never properly deprecated with
trigger_error()
and is used by contrib: http://grep.xnddx.ru/search?text=cacheUntilEntityChanges&filename=That issue also covers
_access_rest_csrf
which was similarly never properly deprecated but is marked for removal in D9 via a @todo comment, but not included in this patch.Comment #15
longwaveShould we throw an exception instead here?
AssetResolverTest::testGetCssAssets() has a comment that refers to this deleted method; this needs updating and the legacy group should also be removed from the test.
Comment #16
Wim Leers#13: Mea culpa on both of those 😭 Those were early Drupal 8 days, and we didn't have established deprecation processes. Still, I wish I'd spotted these months/years ago obviously… So I agree, we can't remove these anymore, they have to stay for the duration of D9.
Comment #17
Gábor HojtsyThese have since had their own issues I think:
Rerolled without those for testing purposes I guess. Looks like we nee an issue for @longwave + @wim.leer's observation to undeprecate those.
Comment #18
longwave#3111504: Properly deprecate AccessResult::cacheUntilEntityChanges() already fixed and committed and #3111506: Properly deprecate _access_rest_csrf route requirement is in progress :)
Comment #20
andypostFiled separate issue for assets #3113756: Remove all @deprecated code from asset component
Comment #21
BerdirReroll and fixing some test fails. Not sure what's up with that bigpipe test yet and the Framework test CSS stuff is also tricky because this does remove the stylesheet-remove thing. I'm not sure if we need to rewrite that test to a library-something.
Comment #23
Berdirstylesheet-remove will be re-deprecated for D10, so removing from this patch.
Comment #25
BerdirReroll after the config issue got in, hopefully just the bigpipe fail left.
Comment #27
longwaveBigPipeTestController did not declare its trusted callbacks correctly, adding the three missing callbacks fixes the test fails locally.
Also, there are a bunch of unused use statements, cleaned up in this patch.
Comment #28
alexpottSo the question is how did we miss the deprecations before. Well I think this is because of how the test works. We're testing what happens when exceptions are thrown and this means we don't get a chance to add the deprecations triggered to the response header. We could probably fix this but given the edge-casey-ness of this I don't think it is worth it.
Comment #29
Berdir@alexpott: not sure if it's the exception, I think it might be that this is being sent after the initial response and at that point we can't add headers anymore to communicate about the deprecations. so this would be a general issue with bigpipe'd lazy builders. But also one that I think we can't do anything about.
Comment #30
longwaveTwo of them are exceptions but
helloOrYarhar
is a normal callback, nothing special. I think as @Berdir says it happens too late for the deprecations to be communicated back, and not immediately sure how we can solve this, or indeed whether it is worth it.Comment #31
alexpott@Berdir @longwave - yep that makes sense - we can no longer change the response headers - and I agree totally not worth trying to fix.
Comment #32
alexpottI reviewed all the code removals and only deprecated code paths are being removed. Any tests that were legacy that are still relevant have been updated to non legacy tests and the others have been removed.
There are a few other deprecated code paths and deprecations that have issues already - TranslationWrapper and Drupal/Core/Entity for example. There are also things like
which can now be
But I think that that is out-of-scope here and could use it's own issue. The same goes for \Drupal\Core\Routing\Router::generate() which is deprecated but we should have a separate issue to change that into an exception of something.
Comment #33
Gábor HojtsyIs it possible to add the respective followup issues? Plus one if we can add a @todo in the issue also :)
Comment #34
Gábor HojtsyAdjusting credits.
Comment #35
alexpottAdded #3114869: \Drupal\Core\Routing\Router::generate() is deprecated but it is on an interface and #3114870: \Drupal\Core\Validation\Plugin\Validation\Constraint\EmailConstraint can be simpler now we use Symfony 4 for #32. I'm not convinced that adding @todo's to the code is adding anything more than stuff to remove later.
Comment #36
Wim LeersNice find :)
And yes, to answer @alexpott's concerns about how this could've been missed before: this is one super special test. Because it's testing low-level details of response streaming. I agree with the conclusion y'all reached.
Comment #38
Gábor HojtsyThanks for opening the followups. @wimleers thanks for double checking the conclusion re the BigPipe test :) Thanks all!