@godotislate

CR https://www.drupal.org/node/3521588 →

OK, got all existing tests passing.

Quick summary of work done here (will update the IS later when I'm less tired):
I ran with the idea of plugins (via attribute discovery) having "dependencies" instead of multiple providers as first mentioned in #8. This led to introducing new methods on the plugin system's AttributeInterface and AttributeBase to get and set the dependencies.

Because there's an existing division between Component AttributeClassDiscovery and Core AttributeClassDiscovery, in that the core class handles the concept of "providers", I split dependency detection between the two. So the dependencies found in the component discovery class are the names of classes, interfaces, and traits in the plugin class's hierarchy. Then in the core discovery class, the providers for classes, interfaces, and traits are derived. The end result is that plugins that have missing provider dependencies (in other words, they rely on assets in uninstalled modules) will be excluded from discovery unless the providers are installed. Keeping track of the classes/traits/interfaces also allows for the flexibility to handle if a plugin class has a non-Drupal class in its hierarchy that is optionally in the codebase. This is not checked now, to avoid having to load classes into memory to check, but it could be done if needed.

The methods on the Attribute class to get/set dependencies makes the dependency information available within its definition for processes that need it, such as migrate_drupal detecting whether a plugin has a dependency on it before requiring a source_module property.

Remaining to do:

• A test that confirms that installing a module causes plugins that have a dependency on it to be discovered (I actually think this exists), but then after that, uninstalling the module then removes the plugin from discovery

Wasn't sure tests were necessary, but it was relatively low lift, so added them.

lgtm

📌 [pp-1] Add tests for passing both identifiers and modules to order against. Active says tests are needed for both OrderAfter and OrderBefore, but I see only a test for OrderAfter added here. Is the OrderBefore test still needed?

PHPCS errors now.

That being said, the validatable config job is showing a warning. Not sure if anything needs to be looked at there.

Tried rerunning the job, got the same warning.

I also tried rebasing, but got this: "Rebase failed: Rebase locally, resolve all conflicts, then push the branch. Try again."

@nicxvan @larowlan Can you confirm that the only "commit-blocking" changes are for the two methods becoming protected instead of public?

With the move to discovery by attributes, dealing with multiple "providers" generally was necessitated since discovery uses reflection and PHP has to parse the class files. With missing providers, this can result in exceptions and/or fatal errors, and this has largely been handled by 📌 Add a fallback classloader that can handle missing traits Active . Remaining part to do for attribute discovery is to filter out plugin definitions that were cached in APCu filecache when the providers (modules) were previously installed but now aren't.

Put up MR with some of this work pulled from attempts in other issues to see what tests pass. Will revisit and address failing tests and check for any outstanding issues later when time permits.

Annotation discovery will be left intact with multiple providers until it is ultimately deprecated and removed.

godotislate → made their first commit to this issue’s fork.

MR is ready https://git.drupalcode.org/project/drupal/-/merge_requests/11957

I think this has been addressed by 📌 Add a fallback classloader that can handle missing traits Active . The InlineBlock plugin class also was converted to attributes there.

Opened coding standards 📌 Traits should always have the suffix "Trait" Active .

godotislate → created an issue.

In #9/#10, there was talk of adding a coding standard for all traits to have names that end in Trait. Is this still needed? Seems like once Drupal's minimum PHP version 8.5, we won't need to do detection specifically for traits.

Did 📌 Add a fallback classloader that can handle missing traits Active to address missing traits instead.

📌 Add an Exit nested layout button Fixed is in, so unblocked.

📌 Add a fallback classloader that can handle missing traits Active is in, so this is unblocked.

Not sure whether this is already tracked anywhere else, but once these changes are in, the Update module overview documentation page → needs an update.

How to deprecated updated:
https://www.drupal.org/about/core/policies/core-change-policies/how-to-d... →

Not really sure why the anchor has a leading "s-".

Add documentation for moving or renaming classes per  https://www.drupal.org/project/drupal/issues/3502882#comment-16033560 📌 Add a classloader that can handle class moves Active

@catch Thanks for those issue links. And yeah, my question is not a blocker here.

One thing about this issue, though, is that maybe there should be at least 1 if not 2 tests?

1. A test that any block with a plugin implementing CacheOptionalInterface is not render cached
2. A test that a page with the language switcher block on it will be cached in the dynamic page cache

The first one at least since an API change is being made to BlockViewBuilder.

1 small nit on MR11571.

And this is a question to make sure I understand the render system correctly, not really a concern:

The language switcher block is not cached on its own, but it is placeholdered with the appropriate cache metadata. Assuming the block is placed globally within a region, because of the placeholdering, the cache contexts do not bubble to dynamic page cache entry, and the cached markup in the dynamic page cache includes the non-replaced placeholder markup.

But if the language switcher blocked were placed in a node layout via layout builder, or rendered inside another block template with twig_tweak or similar, then the node or wrapping block could be rendered cached with all the variations for query strings, etc.? The difference being that in HEAD, with max-age 0 being bubbled, the node or wrapping block would not be cached.

MR is ready.

Can't trigger deprecations for usages ofgetValues() after all, so focusing strictly on getValue().

CR: https://www.drupal.org/node/3521264 →

OK, I think I've gone a bit cross-eyed looking at it, but assuming tests pass once they can run, I think it's RTBC.

I think I might have found some stragglers searching with PHPStorm. I think I filtered out every instance already addressed, and a couple look to be in deprecated code, such as in SSH.php, and some probably are irrelevant or don't really need changing, but documenting here.

For "Update module":

Found occurrences in Project  (54 usages found)
    Usage in comments  (41 usages found)
            core/lib/Drupal/Core/FileTransfer  (1 usage found)
                SSH.php  (1 usage found)
                    6 * The SSH connection class for the update module.
            core/modules/update  (1 usage found)
                update.module  (1 usage found)
                    336 // List of update module cache directories. Do not create the directories if
            core/modules/update/src/Hook  (4 usages found)
                UpdateHooks.php  (4 usages found)
                    193 // Clear all update module data.
                    205 // Clear all update module data.
                    217 // Clear all update module data.
                    229 // Clear all update module data.
            core/modules/update/tests/src/Functional  (1 usage found)
                UpdateMiscTest.php  (1 usage found)
                    11 * Tests general functionality of the Update module.
            core/modules/update/tests/src/Kernel  (6 usages found)
                DevReleaseTest.php  (2 usages found)
                    39 // The Update module's default configuration must be installed for our
                    49 * Sets the current (running) version of core, as known to the Update module.
                UpdateCalculateProjectDataTest.php  (2 usages found)
                    40 // The Update module's default configuration must be installed for our
                    49 * Sets the installed version of core, as known to the Update module.
                UpdateStorageTest.php  (2 usages found)
                    10 * Tests the Update module storage is cleared correctly.
                    24 * Tests the Update module storage is cleared correctly.
            core/tests/Drupal/FunctionalTests  (1 usage found)
                BrowserTestBaseTest.php  (1 usage found)
                    485 // Ensure the update module is not installed.
            core/tests/Drupal/FunctionalTests/Installer  (2 usages found)
                InstallerTest.php  (1 usage found)
                    139 // Ensure the update module is not installed.
                TestingProfileInstallTest.php  (1 usage found)
                    27 * Ensure the Update module is installed.
    Usage in string constants  (12 usages found)
            core/lib/Drupal/Core/Installer/Form  (1 usage found)
                SiteConfigureForm.php  (1 usage found)
                    199 f="@update-module-docs" target="_blank">Update module documentation</a> for details.', ['@update-module-docs' => 'https://www.drupal.org/node/178772']),
            core/modules/update/tests/modules/aaa_update_test  (1 usage found)
                aaa_update_test.info.yml  (1 usage found)
                    3 description: 'Support module for update module testing.'
            core/modules/update/tests/modules/bbb_update_test  (1 usage found)
                bbb_update_test.info.yml  (1 usage found)
                    3 description: 'Support module for update module testing.'
            core/modules/update/tests/modules/ccc_update_test  (1 usage found)
                ccc_update_test.info.yml  (1 usage found)
                    3 description: 'Support module for update module testing.'
            core/modules/update/tests/modules/semver_test  (1 usage found)
                semver_test.info.yml  (1 usage found)
                    3 description: 'Support module for update module testing.'
            core/modules/update/tests/modules/update_test  (1 usage found)
                update_test.info.yml  (1 usage found)
                    3 description: 'Support module for update module testing.'
            core/tests/Drupal/FunctionalTests  (1 usage found)
                BrowserTestBaseTest.php  (1 usage found)
                    486 $this->assertFalse(\Drupal::moduleHandler()->moduleExists('update'), 'The Update module is not installed.');
            core/tests/Drupal/FunctionalTests/Installer  (1 usage found)
                InstallerTest.php  (1 usage found)
                    140 $this->assertFalse($module_handler->moduleExists('update'), 'The Update module is not installed.');

For "Update manager"

Found occurrences in Project  (153 usages found)
    Unclassified  (35 usages found)
            core  (1 usage found)
                MAINTAINERS.txt  (1 usage found)
                    437 Update Manager
    Usage in comments  (102 usages found)
            core/assets/scaffold/files  (3 usages found)
                default.settings.php  (3 usages found)
                    315 * Fallback to HTTP for Update Manager and for fetching security advisories.
                    481 * The Update Manager module included with Drupal provides a mechanism for
                    484 * the Update manager will require the administrator to provide SSH or FTP
            core/lib/Drupal/Core/Updater  (1 usage found)
                Theme.php  (1 usage found)
                    33 * that can conflict on a multi-site installation, since the Update manager
            core/modules/system  (1 usage found)
                system.module  (1 usage found)
                    162 * Because of the Update manager functionality included in Drupal core, there
            core/modules/update  (10 usages found)
                update.authorize.inc  (1 usage found)
                    244 * Since this function is run at such a low bootstrap level, the Update Manager
                update.compare.inc  (1 usage found)
                    203 * returning, or the rest of the Update Manager will break in unexpected ways.
                update.manager.inc  (3 usages found)
                    19 *   The update manager operation we're in the middle of. Can be either 'update'
                    23 *   TRUE if the update manager should continue to the next step in the
                    262 * will eventually be installed, the update manager can transfer files entirely
                update.module  (1 usage found)
                    352 * Deletes stale files and directories from the update manager disk cache.
            core/modules/update/css  (1 usage found)
                update.admin.theme.css  (1 usage found)
                    3  * Styles used by the Update Manager module.
            core/modules/update/src  (4 usages found)
                UpdateRoot.php  (4 usages found)
                    9 * Gets the root path used by the Update Manager to install or update projects.
                    60 * The Update Manager will ensure that project files can only be copied to
                    72 // Normally the Update Manager's root path is the same as the app root (the
                    79 // the Update Manager to be tested) and also ensures that new project files
            core/modules/update/src/Hook  (1 usage found)
                UpdateHooks.php  (1 usage found)
                    265 * update manager does not yet support. See
            core/modules/update/tests/modules/update_test/src/Controller  (2 usages found)
                UpdateTestController.php  (2 usages found)
                    31 * Page callback: Prints mock XML for the Update Manager module.
                    41 *   The project short name the update manager is trying to fetch data for
            core/modules/update/tests/modules/update_test/src/Hook  (1 usage found)
                UpdateTestHooks.php  (1 usage found)
                    72 // environment in which the update manager tests are run).
            core/modules/update/tests/src/Functional  (18 usages found)
                UpdateContribTest.php  (4 usages found)
                    11 * Tests how the Update Manager handles contributed modules and themes.
                    244 * Tests the Update Manager module when one normal update is available.
                    539 // Turn the altering back on and visit the Update manager UI.
                    544 // Turn the altering back off and visit the Update manager UI.
                UpdateMiscTest.php  (1 usage found)
                    61 * Tests the Update Manager module when the update server returns 503 errors.
                UpdateSemverContribBaselineTest.php  (1 usage found)
                    8 * Tests the Update Manager module with a contrib module with semver versions.
                UpdateSemverContribSecurityAvailabilityTest.php  (1 usage found)
                    8 * Tests Update Manager with a security update available for a contrib project.
                UpdateSemverContribTestBase.php  (1 usage found)
                    8 * Base class for Update manager semantic versioning tests of contrib projects.
                UpdateSemverCoreBaselineTest.php  (1 usage found)
                    8 * Tests semantic version handling in the Update Manager for Drupal core.
                UpdateSemverCoreSecurityAvailabilityTest.php  (1 usage found)
                    8 * Tests Update Manager with a security update available for Drupal core.
                UpdateSemverCoreTest.php  (1 usage found)
                    42 * Tests the Update Manager module when the update server returns 503 errors.
                UpdateSemverCoreTestBase.php  (1 usage found)
                    8 * Base class for Update manager semantic versioning tests of Drupal core.
                UpdateSemverTestBaselineTrait.php  (3 usages found)
                    19 * Tests the Update Manager module when no updates are available.
                    49 * Tests the Update Manager module when one normal update is available.
                    122 * Tests the Update Manager module when major updates are available.
                UpdateSemverTestSecurityAvailabilityTrait.php  (2 usages found)
                    13 * Tests the update manager when a security update is available.
                    22 * Tests the Update Manager module when a security update is available.
                UpdateTestBase.php  (1 usage found)
                    78 // Tell the Update Manager module to fetch from the URL provided by
            core/themes/stable9/css/update  (1 usage found)
                update.admin.theme.css  (1 usage found)
                    3  * Styles used by the Update Manager module.
            sites/default  (6 usages found)
                default.settings.php  (3 usages found)
                    315 * Fallback to HTTP for Update Manager and for fetching security advisories.
                    481 * The Update Manager module included with Drupal provides a mechanism for
                    484 * the Update manager will require the administrator to provide SSH or FTP
    Usage in string constants  (16 usages found)
            core/modules/migrate_drupal_ui/tests/src/Functional/d7  (3 usages found)
                MultilingualReviewPageTest.php  (1 usage found)
                    130 'Update manager',
                NoMultilingualReviewPageTest.php  (1 usage found)
                    183 'Update manager',
                Upgrade7Test.php  (1 usage found)
                    220 'Update manager',
            core/modules/update  (3 usages found)
                update.authorize.inc  (1 usage found)
                    221 $session->set('authorize_page_title', t('Update manager'));

Do references to "Update module" need to be changed to "Update Status module" as well? Or is this strictly about making sure there are no references to "Update Manager".

For example, this in locale.translation.inc:

 * For full functionality this function depends on Update module.
 * When Update module is enabled the project data will contain the most recent
 * module status; both in enabled status as in version. When Update module is
 * disabled this function will return the last known module state. The status
 * will only be updated once Update module is enabled.

If not, +1 for RTBC, though it looks like test didn't pass - maybe some kind of gitlab access bug?

Ah right, I'd forgotten that the textarea widget isn't used on text field items. Maybe the unit test is enough, so putting to RTBC.

Pushing back to RTBC per #57.

I actually added 📌 Add test coverage for AttributeClassDiscovery for unexpected class_exists exceptions Active and pushed a commit with the @todo, but we can pick one to close as a dupe.

godotislate → created an issue.

Applied two suggestions. Comment about test coverage is still open, but I wonder if something can be added to core/tests/Drupal/Tests/Component/Plugin/Attribute/AttributeClassDiscoveryCachedTest.php for that.

I would say yes. But it should be already deprecated in 11.x for removal in 12.x so we let contribute & custom code to prepare

Twig is already emitting its own deprecation message. If it ends up that Drupal doesn't move to Twig 4 until Drupal 13 , then overriding the deprecation message for removal in 12 seems premature.

@pdureau 📌 Remove use of deprecated "spaceless" filter in core templates Active has already been committed to 11.x. Only thing outstanding there is whether a port to 10.5.x is required. I believe that addressed #43 already.

Only thing outstanding here is whether Drupal core should provide spaceless to ease contrib/custom template concerns, but if not, then this issue can probably be closed/won't fix.

Because of the browser behavior described in #19, I think it makes sense to include a Browser test after all. I think it'd also make sense to test that the saved value(s) do not exceed DB column sizes, so I think the test could look like this:

• Create an content entity type with a text field (Drupal\text\Plugin\Field\FieldType\TextItem) and set the maxlength storage setting to something low
• Visit entity create form and submit the form where the text field value length should equal the maxlength. This actually should be done three times, with "\r\n", "\n", and "\r" separately included in the text value
• Save should occur without a database exception
• Load the entity from database storage after save and confirm that the text contains "\n" and not "\r\n" or "\r"

A few comments on the MR.

In addition, I think one big thing that is missing is that when the case sensitivity configuration is changed, all the existing redirects will need to have their hashes re-generated and saved. Right now changing the config probably only affects redirect saves (updates and creates) in the future. There probably needs to be some sort of batch process to act on the existing redirects when the configuration is changed. redirect_update_8100() probably can provide some guidance on what the code to update existing redirects should look like.

Per note from alexpott on Slack, pushed commits to clean up some todos around this issue.

Rebased MR against 11.x, and h/t alexpott for changing MR target branch.

Changed to use an inline template test instead. All tests passing now.

godotislate → made their first commit to this issue’s fork.

I think the constructor changes here are in scope because we promote $connection to a property

Oh, right, I missed that bit. Makes sense.

Should probably be converted to constructor promotion, would be in scope to convert the others of this file

Not a blocker, but a couple questions about this:

• Why is property promotion in scope when the constructor was not otherwise changed?
• If property promotion is in scope for ExportStorageManager, shouldn't it also be for ImportStorageTransformer?
• Should the promoted properties be readonly?
• Should the constructor docblocks be removed?

A few comments on the MR.

From the IS, the goal is to remove the additional classes in the selectors since @media (scripting: enabled) is well supported. In which case, seems to me that the selectors within the media query should work well enough as is (and I've just tested to confirm), and the only thing needed is to remove the rules outside the media query.

That reduces the diff size, and the use of .{CLASS}.{SAMECLASS} selectors is used often in core, so I think that part can be left as is.

I have to say, this one in media-library.pcss.css is pretty amusing:
.media-library-item .ajax-progress.ajax-progress.ajax-progress

Looking back at comments from 🐛 Table filter creates jank (layout shift) on page load Fixed :

Original patch in #2 had this:

@media (scripting: enabled) {
  /* Extra specificity to override previous selector. */
  [class] .js-hide {
    display: none;
  }

  .js-show {
    display: block;
  }
}

Suggestion was made for this instead in #8:

Why not .js-hide.js-hide like we do usually when we need to increase specificity?

Manually tested both the views wizard and modules filter and they both look good now. I'm slightly concerned about the use of !important, but I'm not familiar enough with what's going on there to offer any alternatives w/r/t specificity. +1 for RTBC if others are good with the !important.

The views wizard issue is fixed, but the change in MR 11855 causes a regression from 🐛 Table filter creates jank (layout shift) on page load Fixed . The filter is popping in again on /admin/modules when I use Chrome inspector to disable cache and throttle to 3G. This doesn't happen on HEAD.

Screen recording is attached.

There are test failures. Might need rerunning?

Another thing to consider whether it's worth covering for the deprecation: Some form submit methods use $form_state->getValues(), then reference a specific key on the array, e.g. $form_state->getValues()['confirm']; or $values = $form_state->getValues(); $confirm = $values['confirm'];.

Maybe the MR branch needs a rebase? Looks like there was a commit to HEAD that broke TriggeringElementTest, but it was reverted. See 📌 Refactor FormTestClickedButtonForm::buildForm Active #16.

Nit: add readonly to the promoted properties. Otherwise can RTBC.

Added a couple small grammar suggestions.

There are test failures in Drupal\Tests\system\FunctionalJavascript\Form\TriggeringElementTest. I'm guessing unrelated, and the tests need re-running.

Added CR: https://www.drupal.org/node/3519307 →

I added a couple minor comments on the MR after looking again while writing the CR.

Created 📌 Provide a way to deprecate form API array value keys Active per #64.

Added a note that FormStateInterface::getValue() can take nested keys as parameters, so that might need to be accounted for with the #deprecated implementation

godotislate → created an issue.

lgtm

My mistake about the use Drupal\Core\Render\RenderContext; suggestion. It's in the same namespace, so it was unnecessary and flagged by PHPCS.

Also looks like phpstan baseline needs regenerating.

Added a couple comments on the typehints too.

There was some discussion in 📌 Investigate possibilities to parse attributes without reflection Active (#18) whether this actually helps anything.

I looked at this again, and Symfony\Component\DependencyInjection\ContainerBuilder::getReflectionClass() only seems to keep track of reflection classes if the trackingResources class property is set to TRUE, which it isn't for Drupal. And it seems to need Symfony Config to work correctly if it were.

Tempted to set to "Closed (won't fix)", but just going to put back to Active for now in case there's more discussion needed.

Some comments on MR 10795.

Build has PHPCS issue. Also added some comments to the MR.

Applied two more suggestions. Two comments from @quietone are still open, though I added a comment/question to one.

Applied suggestions from @quietone.

I haven't read the full thread but I'm a bit confused why we'd want to show a linked username if the user doesn't have access to view the profile? We'd be showing a link to a 403?

No, the username label would not be linked at all. It would just be text.

The effective difference only applies to users who are viewing their own username's label. Access for the "view" operation is allowed for users with the "access user profiles" permission and for users viewing their own profile. Access for the "view linked label" operation is allowed for users with the "access user profiles" permission and ignores whether the user is viewing their own label. This means that the label is displayed as a link to all users, or displayed as text to all users. This removes the need for the user cache context, and it also makes the label display consistent to all users.

Looking at the HTML5 Standard, the maxlength attribute for a form control is validated by its "API Value".

In the case of textarea elements, the API value and value differ. In particular, newline normalization is applied before the maximum allowed value length is checked (whereas the textarea wrapping transformation is not applied).

And the newline normalization converts "\r\n" and "\r" to "\n". So in PHP, the maxlength should be validated against the submitted value with the same newline normalization.

This likely affects submissions regardless of the user's brower/OS. Also per the HTML5 standard, the submitted form values should have "\n" and "\r" normalized to "\r\n". So the form values in PHP should have "\r\n" exclusively. Testing on Chrome in MacOS, I can validate that this is what I've observed. Putting a breakpoint on these lines in Drupal\Core\Form\FormValidator::performRequiredValidation():

    // Verify that the value is not longer than #maxlength.
    if (isset($elements['#maxlength']) && mb_strlen($elements['#value']) > $elements['#maxlength']) {
      $form_state->setError($elements, $this->t('@name cannot be longer than %max characters but is currently %length characters long.', [
        '@name' => empty($elements['#title']) ? $elements['#parents'][0] : $elements['#title'],
        '%max' => $elements['#maxlength'],
        '%length' => mb_strlen($elements['#value']),
      ]));
    }

If I only hit "return" 20 times in a textarea with #maxlength 20 and submit the form, then the value of $elements['#value'] is "\r\n\r\n\r\n\r\n\r\n\r\n\r\n\r\n\r\n\r\n\r\n\r\n\r\n\r\n\r\n\r\n\r\n\r\n\r\n\r\n", and mb_strlen($elements['#value']) is 40.

I think best way forward is to do the same newline normalization in Drupal\Core\Render\Element\Textarea::valueCallback(), so that the maxlength validation is equivalent on the frontend and backend. I think it also makes sense to store this normalized value, so that there isn't an error if the same character limit is applied on the database column, but it needs to be validated that when the value is retrieved and presented back to browsers, that this doesn't create any issues across browsers/OSes.

Per #6, was there also a regression for '#type' => 'container' that is fixed here? In which case, should there be a test for that as well?

I think this change in DrupalKernel might be the wrong approach:

@@ -1396,7 +1396,7 @@ protected function compileContainer() {
     $container->setParameter('app.root', $this->getAppRoot());
     $container->setParameter('site.path', $this->getSitePath());

-    $container->compile();
+    $container->compile(TRUE);
     return $container;
   }

Looking at the documentation for \Symfony\Component\DependencyInjection\ContainerBuilder::compile(bool $resolveEnvPlaceholders = false):

     * @param bool $resolveEnvPlaceholders Whether %env()% parameters should be resolved using the current
     *                                     env vars or be replaced by uniquely identifiable placeholders.
     *                                     Set to "true" when you want to use the current ContainerBuilder
     *                                     directly, keep to "false" when the container is dumped instead.

Generally the Drupal container does get dumped (and cached), outside of kernel tests. So $resolveEnvPlaceholders should not be set to TRUE per that documentation. In addition, resolving the environment placeholders at compile time would mean that it does not work as Symfony documents:

Use the special syntax %env(ENV_VAR_NAME)% to reference environment variables. The values of these options are resolved at runtime (only once per request, to not impact performance) so you can change the application behavior without having to clear the cache.

Instead, with the placeholders being resolved at compile time, any changes to env variables after the container is cached would not update the container parameter values without a cache rebuild. (If it's fine to support environment variables this way for Drupal, then that difference would need to be documented in the change record.)

And also, as seen in #20 and #28, the extra compile pass resolving the environment variable placeholders is having unexpected side effects.

I think the necessary change to support environment variables is a larger lift: Drupal\Component\DependencyInjection\Container would need to reproduce the environment variable replacement functionality that Symfony\Component\DependencyInjection\Container has, but since the Drupal container is quite different from Symfony's, this effort doesn't look straightforward to me.

Separately, I think changing the Yaml parser to handle constants is out of scope for this issue. Work for that is being done in 📌 Allow parsing and writing PHP constants and enums in YAML files Needs work .

I think the no-needs-review-bot tag deactivates the bot.

IIRC, using \Drupal::service() instead of $this->container->get() addresses issues that surfaced while trying to fix 📌 Memory leak in DrupalKernel when installing modules Active in any test that has a container rebuild. Some of the relevant discussion might've been in Slack, though.

Rebased, removed post update hook per #26. Also autowired the service closure per the original idea in the IS and updated the CR.

Can the work done to this point in the issue fork be turned into an MR (draft, if preferred), to make it the base of any effort going forward?

The tests added for 🐛 Link-widget throws exception when rebuilding a form with an invalid uri Fixed and 🐛 Uncaught exception in link formatter if a link field has malformed data Fixed might be good starting points for testing the logging added here, though perhaps 🐛 Uncaught exception in link formatter if a link field has malformed data Fixed .

Drupal\Tests\language\Kernel\LanguageNegotiatorPluginTest might also provide an example of how to use the BufferingLogger class to assert expected messages were logged.

I am still working on understanding the discussion here, so maybe I do not have it quite right.

Summary of discussion/issue so far:

• The original proposal was to allow any module to provide attribute classes that could add supplementary property values to plugins for plugin types defined by core or other modules. POC of this work is in MR 7793, but the main issue with this approach is that it does not account for the behavior of plugin attribute discovery file cache
• Alternate approach, which is what is currently described in the IS, is for core to provide an attribute class that can be used to set supplementary properties on plugin definitions. This class can be extended in core to apply to plugin types that have object-based definitions (such as entity types) instead of array-based (almost every thing else). The class could also be extended by modules defining the plugin type, but not by other modules. This is done in MR 11218
• One of the plugin subsystem maintainers prefers allowing arbitrary values being set on the plugin attribute itself. This is done in MR 10668. The current small diff might be a little misleading: constructors for every plugin type attribute, or at least, the ones that should allow arbitrary attributes to be set, need to be updated.

@catch has suggested in 📌 Move source_module and destination_module from Migrate to Migrate Drupal Needs work #70 that the file cache issue could be addressed by making the file cache entry keys include the install modules list. Doing so would make the original idea possible.

I also just thought of another possibility to make the original possible once 📌 Add a fallback classloader that can handle missing traits Active gets in, but I'd need to POC it first.

We haven't done a release with the conversion yet, we could still execute it this way and not worry about it.

I agree this seems to be the most straightforward way ahead in that it isn't dependent on resolving blocker issues on the plugin subsystem as a whole.

However, the cache key is set in AttributeDiscoveryWithAnnotations::getFileCacheSuffix and I think we could key that by rootNamespaces which is iirc more or less the module list, that way it will always depend on the combination of modules enabled.

@catch Any concern about possibly having multiple entries, based on combination of modules installed, for just about every file in the plugin directories for every plugin type? Though this would be mitigated once 📌 [meta] Lots of non-plugin PHP classes in plugin directories Active is addressed.

This could be isolated to migrate source plugin discovery, because it has its own classes AttributeClassDiscoveryAutomatedProviders and AttributeDiscoveryWithAnnotationsAutomatedProviders. On the other hand, it's probably not ideal to make migrate source discovery any more special than it already is.

Getting ✨ Allow attribute-based plugins to discover supplemental attributes from other modules Active would be great, but I think it's at an impasse for now on what approach to go forward with.

Another idea would be to remove the source_module property from the MigrateSourceattribute, and use

hook_migrate_source_info_alter()

implementations to add the source_module property to the relevant definitions. This would be similar to the approach taken for the Navigation module in 📌 Provide a way for other modules to flag block plugin implementations as 'navigation safe' Active , where hook_block_alter() implementations can be used to add the allow_in_navigation property to block plugin definitions. For migrate_drupal source plugins, this probably could be done as one single alter hook in migrate_drupal for all the relevant plugins throughout core modules.

I don't think it's necessary to add an other comment for additional services that need to be re-resolved. In that case, only the comment on the test is needed because it's really connected to this behavior.

Sorry if this was unclear, but yes, this is the only comment that was needed. My previous post referred to my suggestion on the MR.

Concerning the documentation, I would agree to add a comment but I don't know where to put it. Does the sites/default/default.services.yml file seem right to you (After that, the content of the documentation is the same as for Symfony)?
I don't think we have access to the technical documentation repository to add a section.

The ask isn't for more documentation in code. You can follow these instructions on how to add a change record to a Drupal issue → . For an actual example, look at the change record documenting autowired services → being added to Drupal.

Then I created the MigrateDrupalSource attribute class, extending MigrateSource, and moved the source_module property to the new class.

The unfortunate issue with an attribute being defined in a module separate from the one that has the the main attribute is this:

• Site has the migrate and my_module modules installed, but not migrate_drupal
• On cold caches, migrate source plugin discovery iterates through all the plugin directories and picks up definitions using the MigrateSource attribute. The MigrateDrupalSource attribute on any class is ignored as a comment, since the attribute class does not exist with the module uninstalled. This all works as expected without error
• However, once discovery iterates through a directory, the definition data for all the files in that directory are saved in a file cache in APCu, and the entries in that file cache are only invalidated by change in file mtime
• So, assuming you have a migrate source plugin Drupal\my_module\Plugin\migrate\source\MyMigrateDrupalSource that uses the
  

  MigrateDrupalSource

  attribute, the entry associated with the class saved in the file cache will be NULL

• If you install migrate_drupal, the file cache data is not invalidated, so the
  

  NULL

  value for MyMigrateDrupalSource is retrieved from cache. MyMigrateDrupalSource.php doesn't get parsed for the attribute data, and the definition is not discovered

There are some attempts to deal with a very similar problem in ✨ Allow attribute-based plugins to discover supplemental attributes from other modules Active .

One comment: should document the additional services/parameters that need to be re-resolved, accounting for the additional compiler pass.

Is this being ported to 10.5.x? Just noticed it's been in "Patch (to be ported)" for a couple months now.

Note that this is probably soft-blocked by 📌 Hook ordering across OOP, procedural and with extra types Active , or at least there will likely be a giant merge conflict resolution needed between the two.

So IF $container->registerAttributeForAutoconfiguration is used, i see no rational reason for performance fear left.

So while core does not use $container->registerAttributeForAutoconfiguration, with

autoconfigure: true

and several interfaces registered for autoconfiguraiotn in CoreServiceProvider::register() with $container->registerForAutoconfiguration() (no Attribute), it does turn out that at compile time all core services are iterated over and reflected.

Given this, @ghost of drupal past mentioned that all service classes are being loaded into memory at compile time anyway, so iterating with reflection on these classes to discover class-level `Hook` attributes has a negligible affect performance. @nicxvan also ran performance tests iterating reflection through each of a class's methods, and the hit on performance was not significant even with the number of methods in the class in 100,000s. Without an additional performance concern, it seems like a good idea to go forward.

Haven't completely found out why, but the DrupalKernel changes lead to the addition of Symfony\Component\DependencyInjection\Compiler\ResolveEnvPlaceholdersPass, and that makes it necessary to re-resolve the `%container.modules%` parameter to ModuleHandler in the test as well.

diff --git a/core/modules/ckeditor5/tests/src/Kernel/CKEditor5PluginManagerTest.php b/core/modules/ckeditor5/tests/src/Kernel/CKEditor5PluginManagerTest.php
index 30cd8fe3b60..23decc3ba8f 100644
--- a/core/modules/ckeditor5/tests/src/Kernel/CKEditor5PluginManagerTest.php
+++ b/core/modules/ckeditor5/tests/src/Kernel/CKEditor5PluginManagerTest.php
@@ -167,7 +167,9 @@ private function mockModuleInVfs(string $module_name, string $yaml, array $addit
     // The exception to the above elegance: re-resolve the '%app_root%' param.
     // @see \Symfony\Component\DependencyInjection\Compiler\ResolveParameterPlaceHoldersPass
     // @see \Drupal\Core\DrupalKernel::guessApplicationRoot()
-    $container->getDefinition('module_handler')->setArgument(0, '%app.root%');
+    $container->getDefinition('module_handler')
+      ->setArgument(0, '%app.root%')
+      ->setArgument(1, '%container.modules%');

     // To discover per-test case config schema YAML files, work around the
     // static file cache in \Drupal\Core\Extension\ExtensionDiscovery. There is

benjifisher → credited godotislate → .

These are probably tricky enough that we should do them in multiple issues, we could maybe re-title this one to just cron and open a new meta for the others?

Cloned to 📌 [meta] Replace lazy service proxy generation with service closures Active as the meta and retitled here.

godotislate → created an issue.

Also a general question - is this the last lazy class in core?

Checking now, looks like there are still several others. Not sure on the history of this issue for why only cron is being converted. Tagging for an IS update. Either this issue should be a meta, with individual issues addressing each proxy class, or maybe an update on why only cron is being converted.

LGTM

No, this is just a very new rule, was not enforced before.

Oh, interesting. I could've sworn the 80-character limit for comments was an old rule, but good to know.

Added comments to the MR with a few thoughts.

Is it weird that the bot is picking up PHPCS issues that the build is not?

smustgrave → credited godotislate → .

FYI for future reference, I think using __METHOD__ can save having to type ConfirmFormBase::getFormName(), etc.

Commented on MR with suggestion to meet CS line length issue flagged by NR bot.

NR bot hopefully appeased.

Well if it's meant to be permanent in Drupal shouldn't that be announced? Also if permanent SpacelessBCExtension is the BC part needed?

There probably should be a CR for what the support policy will be for spaceless, but there needs to be agreement/direction on what that should be. There are questions about whether this should be done at all, or Drupal should emit a its own deprecation message, or if it's long-term support.

I don't think that SpacelessBCExtension specifically needs a CR, since it's internal implementation details that's not API.