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.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Berdir created an issue. See original summary.

Berdir’s picture

Status: Active » Needs review
FileSize
72.47 KB

Status: Needs review » Needs work

The last submitted patch, 2: core-bc-removal-3104307-2.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

andypost’s picture

+++ b/core/lib/Drupal/Core/Database/Driver/mysql/Schema.php
@@ -486,30 +486,6 @@ public function dropField($table, $field) {
-  public function fieldSetDefault($table, $field, $default) {

Database changes using #3097879: Remove all @deprecated code in \Drupal\Core\Database

Berdir’s picture

Yeah, 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()

Wim Leers’s picture

this is for later anyway when the other components are done

Could you point to the specific issues this should be postponed on?

Berdir’s picture

database 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.

Wim Leers’s picture

Title: Remove BC layers in various Drupal\Core components » [PP-2] Remove BC layers in various Drupal\Core components
Status: Needs work » Postponed

#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 :)

Wim Leers’s picture

Title: [PP-2] Remove BC layers in various Drupal\Core components » [PP-1] Remove BC layers in various Drupal\Core components
Berdir’s picture

In 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.

Berdir’s picture

Fun, nightwatch is failing because the setup stuff is using the FunctionalTestSetupTrait without setting $defaultTheme, that triggers deprecation messages that aren't reported?

Berdir’s picture

Title: [PP-1] Remove BC layers in various Drupal\Core components » Remove BC layers in various Drupal\Core components
Status: Postponed » Needs review
Parent issue: #3104306: Remove BC layers in the extension component » #2716163: [META] Remove deprecated classes, methods, procedural functions and code paths outside of deprecated modules on the Drupal 9 branch
FileSize
99.13 KB
longwave’s picture

+++ b/core/lib/Drupal/Core/Access/AccessResult.php
@@ -278,37 +278,6 @@ public function cachePerUser() {
-  public function cacheUntilEntityChanges(EntityInterface $entity) {
-    return $this->addCacheableDependency($entity);
-  }

In #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.

Status: Needs review » Needs work

The last submitted patch, 12: core-bc-removal-3104307-12.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

longwave’s picture

  1. +++ b/core/lib/Drupal/Core/Plugin/PluginDependencyTrait.php
    @@ -60,10 +60,6 @@ protected function getPluginDependencies(PluginInspectionInterface $instance) {
    -      else {
    -        @trigger_error('Declaring a dependency on an uninstalled module is deprecated in Drupal 8.7.0 and will not be supported in Drupal 9.0.0.', E_USER_DEPRECATED);
    -        $dependencies['module'][] = $provider;
    -      }
    

    Should we throw an exception instead here?

  2. +++ b/core/lib/Drupal/Core/Theme/ActiveTheme.php
    @@ -206,40 +180,6 @@ public function getLibraries() {
    -  public function getStyleSheetsRemove() {
    

    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.

Wim Leers’s picture

#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.

Gábor Hojtsy’s picture

Status: Needs work » Needs review
FileSize
92.86 KB

These have since had their own issues I think:

$ git apply --index core-bc-removal-3104307-12.patch 
error: patch failed: core/lib/Drupal/Core/Access/AccessResult.php:278
error: core/lib/Drupal/Core/Access/AccessResult.php: patch does not apply
error: core/lib/Drupal/Core/Field/Plugin/migrate/field/Email.php: does not exist in index
error: core/lib/Drupal/Core/Field/Plugin/migrate/field/d7/EntityReference.php: does not exist in index
error: core/lib/Drupal/Core/Field/Plugin/migrate/field/d7/NumberField.php: does not exist in index
error: patch failed: core/tests/Drupal/Tests/Core/Access/AccessResultTest.php:969
error: core/tests/Drupal/Tests/Core/Access/AccessResultTest.php: patch does not apply

Rerolled without those for testing purposes I guess. Looks like we nee an issue for @longwave + @wim.leer's observation to undeprecate those.

Status: Needs review » Needs work

The last submitted patch, 17: 3104307-17.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

andypost’s picture

Berdir’s picture

Status: Needs work » Needs review
FileSize
85.67 KB
15.42 KB

Reroll 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.

Status: Needs review » Needs work

The last submitted patch, 21: 3104307-21.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

Berdir’s picture

Status: Needs work » Needs review
FileSize
76.25 KB
9.42 KB

stylesheet-remove will be re-deprecated for D10, so removing from this patch.

Status: Needs review » Needs work

The last submitted patch, 23: 3104307-23.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

Berdir’s picture

Status: Needs work » Needs review
FileSize
64.67 KB

Reroll after the config issue got in, hopefully just the bigpipe fail left.

Status: Needs review » Needs work

The last submitted patch, 25: 3104307-25.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

longwave’s picture

BigPipeTestController 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.

alexpott’s picture

So 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.

Berdir’s picture

@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.

longwave’s picture

Two 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.

alexpott’s picture

@Berdir @longwave - yep that makes sense - we can no longer change the response headers - and I agree totally not worth trying to fix.

alexpott’s picture

Status: Needs review » Reviewed & tested by the community

I 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

    // Since Symfony 4.1, the 'mode' property is used, for previous versions the
    // 'strict' property. If the 'strict' property is set,
    // \Symfony\Component\Validator\Constraints\EmailValidator will trigger
    // a deprecation error, so only assign a value for versions of Symfony
    // < 4.2. This compatibility layer can be removed once Drupal requires
    // Symfony 4.2 or higher in https://www.drupal.org/node/3009219.
    if (property_exists($this, 'mode')) {
      $default_options = ['mode' => 'strict'];
    }
    else {
      $default_options = ['strict' => TRUE];
    }
    $options += $default_options;

which can now be

$options += ['mode' => 'strict'];

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.

Gábor Hojtsy’s picture

Issue tags: +Needs followup

Is it possible to add the respective followup issues? Plus one if we can add a @todo in the issue also :)

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.

Gábor Hojtsy’s picture

Adjusting credits.

alexpott’s picture

Wim Leers’s picture

BigPipeTestController did not declare its trusted callbacks correctly, adding the three missing callbacks fixes the test fails locally.

Nice 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.

  • Gábor Hojtsy committed d1c44dd on 9.0.x
    Issue #3104307 by Berdir, longwave, Gábor Hojtsy, Wim Leers, alexpott,...
Gábor Hojtsy’s picture

Status: Reviewed & tested by the community » Fixed

Thanks for opening the followups. @wimleers thanks for double checking the conclusion re the BigPipe test :) Thanks all!

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.