Deprecate block_place module for removal in Drupal 9

So the module is hidden from the UI, but there's no deprecation messages surfaced. I think in 8.8.x we should:

1. Create a https://www.drupal.org/project/block_place contrib project from the 8.8.x codebase of the module. The project page should indicate that it has no further development and will not have any releases for Drupal 8 or 10, and recommend Layout Builder instead.
2. Add a CR announcing the deprecation and recommending sites switch to Layout Builder, but indicating that the contrib project will be available as a fallback if needed.
3. Add a status report info message that the module is deprecated and will be moved to contrib in Drupal 9. Recommend Layout builder.
4. Add an install warning message as above.
5. Mark all the PHP code, JS code, and libraries deprecated. I think that also means the module's own test suite will need to be marked legacy.
6. Mention that the module is deprecated in its description, so sites that are installed see it on the modules page too.
7. File and/or link a related issue to allow modules to be deprecated wholesale the way that libraries are. That's a should-have, but not a hard requirement IMO, since blasting deprecations all over the whole module should be sufficient to make sites fail Upgrade Status hard if they're using it.
8. When 9.0.x opens, rm -rf the thing.

Would this be enough for deprecating the module, or do the tests also be marked as deprecated?

The namespace https://www.drupal.org/project/block_place was taken a few days ago (but git does not have any commits as of now), I've contacted the current module owner.

This is a great start; thanks @rpsu!

I wonder if the person who took the namespace was also thinking to contribute here. We definitely need the namespace for a safe upgrade path for core.

A few suggestions and next steps:

1. For the deprecation messages, we should add a change record draft that documents the same thing in a little more detail, with instructions for sites or modules that depend on block_place. Then it gets simply added on with an @see after the @deprecated: https://www.drupal.org/core/deprecation#how
2. Then we should add @trigger_error() inside the deprecated code (as described in the handbook page above). This will help sites and modules identify that deprecated code is being used at runtime or during test runs.
3. Then, the @trigger_error() should start causing block_place tests to fail. We mark the tests as legacy tests to get them to pass again.
4. We should also be able to deprecate the module's libraries, as described here: https://www.drupal.org/node/3064022
5. And finally, just a docs update in the module's info.yml, hook_help(), and so on with the same messages.

Thanks so much for getting this issue rolling.

I can't find policy about deprecation of modules but there's example in core's entity_reference module

+++ b/core/modules/block_place/src/Plugin/DisplayVariant/PlaceBlockPageVariant.php
@@ -80,6 +97,11 @@ public static function create(ContainerInterface $container, array $configuratio
    * {@inheritdoc}
+   *
+   * @deprecated in Drupal 8.8.x and will be removed before Drupal 9.0.0.

No need for that, the trigger_error should be added to start of file

Here are the references for classes: https://www.drupal.org/core/deprecation#how-classes
And for files of procedural code: https://www.drupal.org/core/deprecation#how-file

Thanks @andypost!

I did not get any response yet to my direct contact of the current module owner, so I created an issue in the empty module's issue queue:
#3081451: Project namespace collision with core deprecation module

I think it'd be good to wait for that module's maintainer to respond before writing the deprecation messages - I sort of hope we could state something like
'The Place blocks -module is deprecated in drupal:8.8.0 and is removed from drupal:9.0.0. Use of Layout builder is recommended, or using contrib module in https://www.drupal.org/project/block_place . See http://drupal.org/node/the-change-notice-nid

https://git.drupalcode.org/project/block_place has now branch 9.x-1.x with the almost identical clone of what is currently in 8.8.x

Draft change record to be reviewed https://www.drupal.org/node/3081957

The method deprecations are now removed and (only) the @trigger_error are added in files. Also updated the module description with info about the module is being deprecated.

Should the deprecation message also include a link to the contrib module for Drupal 9?

Uploading correct patch & interdiff.

I decided to add the deprecation msg to class docblocks, too.

Apparently creating branch for D9 is not the way here, but rather (re) create the block_place module only with branch 8.x-1.x (which will take precedence if present over the core's block_place).

Copying here what @mixologic wrote about branching and moving the module to contrib space:

Composer doesnt allow for multiple things to share the same namespace, so, in effect a contrib module and a core module should not share the same name, because composer's dependency mapping will break. normally what would happen is the contrib module would be forced to use the drupal/projectname-modulename pattern for the namespace, because core already owns the drupal/projectname namespace.

The problem will be if there are other contrib modules that have a dependency on block_place.

I looked at it for the simpletest module, and, since no contrib has a constraint on a specific version of simpletest, then we can move it to contrib, and make sure that it gets the drupal/simpletest namespace, because then modules will end up with a dep on the contrib module instead of the core module, and everything should be happy.

Same goes for block_place. If there are contrib that rely on a specific version then we might have to figure something out. - good news is that I just did a query and it doesnt appear as though there are any contrib modules relying on the block_place module, which means I can add it to the exception list for when we remove it, so that if people need to add it with composer they can.

tl;dr; - block_place should be fine as is. Do not create a 9.x branch, please create an 8.x- branch for now.

1. Create a https://www.drupal.org/project/block_place contrib project from the 8.8.x codebase of the module. The project page should indicate that it has no further development and will not have any releases for Drupal 8 or 10, and recommend Layout Builder instead.

This is now done apart from the module not having yet releases. Once I've run the tests with d.o. test infrastructure it'll land, too.

Deprecations trigger_error()'s added to
- after namespace in class files
- class dockblock
- class constructor
- beginning of .module file
- services file
- info.yml file

All of them are pointing to the change record node.

I'm not sure how to deal with this - tests are failing because of all the deprecation notices added by the patch.

Patch itself seems to be doing what it is supposed to do, and has no errors. however if this patch gets committed, does it not mean that core tests start failing, too?

https://www.drupal.org/core/deprecation#what-not says:

What we can not deprecate
Do not deprecate test classes. Any test that could be considered deprecated should be removed or modified to be relevant.

Tests which exercise deprecated code can be annotated with @group legacy in order to avoid deprecation notices during testing, but a follow-up issue should be filed in order to fix such tests to not exercise deprecated code.

So I think Place Block tests should not be deprecated. But I hesitate to add @group legacy to the currently failing tests since I believe it would hide other test failures, too?

> But I hesitate to add @group legacy to the currently failing tests since I believe it would hide other test failures, too?

That's a risk we have to take and it's OK because it doesn't matter if the core module uses deprecated code.

So yes, you have to add that. It's the same with our upgrade test, they all also have @group legacy.

I see, thanks @Berdir for the input.

Forgot that naturally also Place Block test need to have @group legacy, too.

These aren't done yet:

3.
Add a status report info message that the module is deprecated and will be moved to contrib in Drupal 9. Recommend Layout builder.

4. Add an install warning message as above.

Added status-report -page info, installation time warning (visible only when installing with drush/drupal console, as the module is hidden in /admin/modules -page) and made the messages/errors/info be more consistently formatted.

This may not be the right place to ask this, but I feel I must. I don't understand how Layout Builder supersedes the functionality. When a block (let's say a menu block) has to be placed across nearly all pages (let's say all but landing page content type) and this includes pages created by views panels (yes, it still exists), or modules, how does layout builder solve that issue? Placing blocks in a region across all of these type of pages has been in use for a long time. I thought Block Place got it right where it allowed you to place the same block in multiple places with the potential for different configuration.

Umm. Libraries need still deprecation info, see comment comment #7

Umm. Libraries need still deprecation info, see comment comment #7

https://www.drupal.org/node/3064022

Including the deprecation of *.libraries.yml.

From @Berdir in Slack, related to #33:

that's the part that I don't quite get, because LB is about controlling the layout of an entity? while block_place is about placing blocks in theme regions. LB would need to completely replace block.module aka provide its own DisplayVariant to take over the whole page and I thought you still need panels/page manager for that use case?

Following up on #33 and #37, this project is the recommended contrib replacement if the blocks specifically need to be placed in a theme region:
https://www.drupal.org/project/lb_everywhere

That functionality won't be in core before (probably) 9.1, but it is the future-compatible solution.

I think we should add this information to the change record and release note, but don't need to mention it in the deprecation messages since it's not techncially in core yet.

lb_everywhere is in a bullet point in the change record now (I just went to add it and found it). Seems sensible to put it there but not in the core message - let people read the change record.

Patch looks ready to me.

That's a real test failure due to the help topics commit and the deprecation notices introduced here.

Marked HelpTopicsSyntaxTest::testHelpTopics as legacy, and added it also to the "fix later" issue #3084471: Remove/fix legacy tests after Place Blocks -module removal

Assuming this is green, back to RTBC.

The 9.x follow-up to un-@legacy those tests is a good approach IMO.

Patch went from 17kb to 759 bytes, mix up?

Reviewing the previous patch.

+++ b/core/modules/help_topics/tests/src/Functional/HelpTopicsSyntaxTest.php
@@ -29,6 +29,8 @@ class HelpTopicsSyntaxTest extends BrowserTestBase {
+   * @group legacy

I don't think this is the correct approach, I'd prefer to exclude block_place from the test, see \Drupal\Tests\help_topics\Functional\HelpTopicsSyntaxTest::testHelpTopics it enables all core modules it finds, I think we should just remove block_place from that list

The previous patch looks good though, so just this change and fix to the actual patch

Thanks!

Very much a mixup, sorry.

If the Help Topics test is not marked as legacy, it is handled differently than all other tests (see #3084471: Remove/fix legacy tests after Place Blocks -module removal) - is that intentional?

The plan is to fix the those tests marked by this deprecation as legacy once module is removed (in branch 9.0.x).

I'll fix the patch once the intended approach is confirmed.

New patch, not marking HelpTopics test as legacy but simply not enabling the Place Block module for the tests.

+++ b/core/modules/help_topics/tests/src/Functional/HelpTopicsSyntaxTest.php
@@ -36,6 +36,11 @@ public function testHelpTopics() {
+    // @todo Temporary exclusion of a deprecated Place block -module.

nit: s/Place block -module/Place block module/

Otherwise I'd mark this RTBC.

We need an explicit follow-up to un-legacy the tests that are legacied here.

FIxes #48

#48 is addressed so back to RTBC.

Committed f2955eb and pushed to 9.0.x. Thanks!

c/p to 8.8.x and 8.9.x

I wish I had seen this before it got committed, I would have objected to all of these migration tests being marked @legacy. They are the main tests of several code paths in the migration system and now they will not alert us if any code in those paths becomes deprecated. I realize we are essentially done adding deprecations in D8, but I really hope this doesn't come back to bite us later.

Discussed this with @mikelutz and @xjm in slack.

We are in agreement that the loss of coverage for Migrate module is not ideal and have decided to roll this back, and work on a more appropriate fix, which is to exclude block place from those tests, like we did for the help topics one.

Reverting from all three branches

Here's a start on that, untested.

oh, working in parallel..

this is an alternative approach - $deprecated_modules is a class variable and used as such to exclude some (one) modules.

Also I duplicated tests to 2, where one is marked as legacy and does NOT skip block_place, and the original one skips it.

I feel this might be an overkill solution, but on the other hand it is extendable.

1. +++ b/core/modules/migrate/tests/src/Kernel/Plugin/MigrationProvidersExistLegacyTest.php
   @@ -0,0 +1,20 @@
   +class MigrationProvidersExistLegacyTest extends MigrationProvidersExistTest {
   

   the legacy versions should start with Legacy (i.e. LegacyMigrationProvidersExistTest, not MigrationProvidersExistLegacyTest)

2. +++ b/core/modules/migrate_drupal_ui/tests/src/Kernel/MigrationLabelExistTest.php
   @@ -37,4 +40,19 @@ public function testLabelExist() {
   +  /**
   +   * Enables all modules, except the ones marked as deprecated.
   +   */
   +  public function enableAllModules() {
   +    // Install all available modules.
   +    $module_handler = $this->container->get('module_handler');
   +    $modules = $this->coreModuleListDataProvider();
   +    $modules_enabled = $module_handler->getModuleList();
   +    $modules_to_enable = array_keys(array_diff_key($modules, $modules_enabled));
   +    $modules_to_enable = array_diff($modules_to_enable,
   +                                    $this->deprecated_modules);
   +    $this->enableModules($modules_to_enable);
   +
   +  }
   +
   

   Some of the reused logic should go into a trait, if we have to explictly name block_place as the only deprecated module, that should be defined once somewhere in that trait.

fwiw https://www.drupal.org/project/drupal/issues/3062281#comment-13305130 is just changing the migration trait that provides the list of all modules to remove block_place

I wrote a new trait (did not dare to touch the one @catch had altered in his patch). I'm happy to go either way but the FileSystemModuleDiscoveryDataProviderTrait is not used in all the tests. Core modules seem to have their own ways to get list of core modules in many cases.

Also test classes using the block_place module are now renamed.

Umm.probaly a brainfart in the trait, block_place module seems to be enabled after all.

New patch, hopefully less brainfarts.

maybe now

Coding standard fixes, mainly (not meeting *all* of what phpcs wines about, though.

Locally tests are passing fine in all the changed/added tests, but Drupal CI still displays errors?

Remove test code that sneaked into the previous patch. Fix module array cleaning when module machine name is the key.

Looks like help_topics module has disappeared here in between.

Xjm @Slack

Do what I said, but un-deprecating event subscribers seems fine if that makes tests pass

this will likely help the need to create a set of Legacy-tests, and the need for the trait.

Trying out the patch here: due to upstream changes I can't rebase / apply previous patches anymore.

"undeprecated" the BlockPlaceEventSubscriber to avoid other core modues testing in a very greedy way to trigger deprecation errors where.

I expect this to need more work but I just wanna see the failing tests now.

checking how CI likes this now, and if ti does not will start removing @trigger_error's starting i Drupal\block_place\Plugin\DisplayVariant\PlaceBlockPageVariant

remove debuggers, correct one boolean and keeping thumbs up (but most likely need to actually comment out the @trigger_warning() from Drupal\block_place\Plugin\DisplayVariant\PlaceBlockPageVariant

Commented out the @trigger_error() also from the Drupal\block_place\Plugin\DisplayVariant\PlaceBlockPageVariant::_constructor(), now this will be finally passing.

// This is disabled to avoid deprecation warnings in
// Drupal\Tests\migrate_drupal\Kernel\d7\ValidateMigrationStateTest and
// Drupal\Tests\migrate_drupal\Kernel\d6\ValidateMigrationStateTest
// due to issues finding where exactly the test enables all (?) core modules.

I'll update the follow up #3084471: Remove/fix legacy tests after Place Blocks -module removal with the classes that have been added or altered once the tests have passed.

Cleaning up notation (coding standards) and removing the last final @deprecated and assuming it is the last possible warning triggerer.

+++ b/core/modules/block_place/block_place.module
@@ -3,8 +3,20 @@
+@trigger_error('The ' . __NAMESPACE__ . '\PlaceBlockPageVariant is
+  deprecated in drupal:8.8.0 and will be removed from drupal:9.0.0. Using Layout
+  Builder (available in Drupal 8.7.0) module is recommended, or alternatively ¶
+  you may use the contrib module Place Blocks. See ¶
+  https://www.drupal.org/node/3081957.', E_USER_DEPRECATED);

This mentions PlaceBlockPageVariant in the .module file, not sure why? I think this is the cause of the remaining fails.

Removing the @trigger_error from the .module -file (with some coding style - spacing - fixes).

It this passes (the .module trigger message error was erroneous) I'll try to see if the PlaceBlockPageVariant can actually have the deprecations declared.

I'll try to see if the PlaceBlockPageVariant can actually have the deprecations declared.

Great work on this! Yay, green patch.

I should probably should put the RM tag on just to make sure we double-check the disruptions once we get to the final versions. We might also want to mention this in the release notes given that it's a whole module, so tagging for that.

1. +++ b/core/modules/block_place/block_place.install
   @@ -0,0 +1,36 @@
   + */
   +function block_place_requirements($phase) {
   +  $requirements = [];
   +  // Notify about the module being deprecated.
   +  if ($phase == 'runtime') {
   +    $requirements['block_place'] = [
   +      'title' => t('Place blocks'),
   +      'severity' => REQUIREMENT_INFO,
   +      'description' => t('Place Blocks (Core, Experimental) is deprecated in Drupal 8.8.0 and  will be removed in Drupal 9.0.0. Using Layout Builder (available in Drupal 8.7.0) module is recommended, or alternatively you may use the contrib module <a href=":contrib-link">Place Block</a>. See <a href=":change-record">the change record</a>.', [
   +        ':change-record' => 'https://www.drupal.org/node/3081957',
   +        ':contrib-link' => 'https://www.drupal.org/project/block_place',
   +      ]),
   +    ];
   +  }
   +
   +  return $requirements;
   

   +1

2. +++ b/core/modules/block_place/block_place.install
   @@ -0,0 +1,36 @@
   +
   +/**
   + * Implements hook_install().
   + */
   +function block_place_install() {
   +  \Drupal::messenger()->addWarning(t('Place Blocks (Core, Experimental) is deprecated in Drupal 8.8.0 and  will be removed in Drupal 9.0.0. Using Layout Builder (available in Drupal 8.7.0) module is recommended, or alternatively you may use the contrib module <a href=":contrib-link">Place Block</a>. See <a href=":change-record">the change record</a>.', [
   +    ':change-record' => 'https://www.drupal.org/node/3081957',
   +    ':contrib-link' => 'https://www.drupal.org/project/block_place',
   +  ]));
   +}
   

   Also +1

3. +++ b/core/modules/block_place/block_place.install
   @@ -0,0 +1,36 @@
   diff --git a/core/modules/block_place/block_place.libraries.yml b/core/modules/block_place/block_place.libraries.yml
   
   diff --git a/core/modules/block_place/block_place.libraries.yml b/core/modules/block_place/block_place.libraries.yml
   index 0671252032..a257695ab5 100644
   
   index 0671252032..a257695ab5 100644
   --- a/core/modules/block_place/block_place.libraries.yml
   
   --- a/core/modules/block_place/block_place.libraries.yml
   +++ b/core/modules/block_place/block_place.libraries.yml
   
   +++ b/core/modules/block_place/block_place.libraries.yml
   +++ b/core/modules/block_place/block_place.libraries.yml
   @@ -3,9 +3,11 @@ drupal.block_place:
   
   @@ -3,9 +3,11 @@ drupal.block_place:
      css:
        theme:
          css/block-place.css: {}
   +  deprecated: The "%library_id%" asset is deprecated in drupal:8.8.0 and will be removed in drupal:9.0.0. Using Layout Builder (available in drupal:8.7.0) module is recommended, or alternatively you may use the contrib module Place Blocks. See https://www.drupal.org/node/3081957.
    
    drupal.block_place.icons:
      version: VERSION
      css:
        theme:
          css/block-place.icons.theme.css: {}
   +  deprecated: The "%library_id%" asset is deprecated in drupal:8.8.0 and will be removed in drupal:9.0.0. Using Layout Builder (available in drupal:8.7.0) module is recommended, or alternatively you may use the contrib module Place Blocks. See https://www.drupal.org/node/3081957.
   diff --git a/core/modules/block_place/block_place.module b/core/modules/block_place/block_place.module
   

   I'm not sure if we really need to individually mark all these things deprecated, but also it doesn't do any harm and may help in the case some code relies on these.

4. +++ b/core/modules/block_place/block_place.module
   @@ -3,8 +3,26 @@
    
   +/*
   + * This is disabled to avoid deprecation warnings in
   + * Drupal\Tests\migrate_drupal\Kernel\d7\ValidateMigrationStateTest and
   + * Drupal\Tests\migrate_drupal\Kernel\d6\ValidateMigrationStateTest
   + * due to issues finding where exactly the test enables all (?) core
   + * modules.
   + */
   +// @trigger_error('The Block Place -module is deprecated in drupal:8.8.0
   +// and will be removed from drupal:9.0.0. Using Layout Builder (available in
   +// Drupal 8.7.0) module is recommended, or alternatively you may use the contrib
   +// module Place Blocks. See https://www.drupal.org/node/3081957.',
   +// E_USER_DEPRECATED);.
   

   I think we can remove this whole section rather than commenting it out.

5. +++ b/core/modules/block_place/src/Plugin/DisplayVariant/PlaceBlockPageVariant.php
   @@ -2,6 +2,12 @@
    
   +@trigger_error('The ' . __NAMESPACE__ . '\PlaceBlockPageVariant is
   + deprecated in drupal:8.8.0 and will be removed from drupal:9.0.0. Using
   + Layout Builder (available in Drupal 8.7.0) module is recommended, or
   + alternatively you may use the contrib module Place Blocks. See
   + https://www.drupal.org/node/3081957.', E_USER_DEPRECATED);
   +
   

   Given we're deprecating in the constructor, I don't think we also need this one.

New patch fixing the mentioned issues in #95 + one coding standards addition (adding relevant @see URL after @deprecated in docblock)

Looking good!

Just a couple nits and an idea that might simplify things and remove duplicate documentation.

1. +++ b/core/modules/block_place/block_place.module
   @@ -47,7 +57,6 @@ function block_place_toolbar() {
   -
   
   @@ -78,7 +87,7 @@ function block_place_toolbar() {
   -   ],
   +    ],
   

   These changes seem unrelated

2. +++ b/core/modules/block_place/src/EventSubscriber/BlockPlaceEventSubscriber.php
   @@ -36,6 +36,7 @@ class BlockPlaceEventSubscriber implements EventSubscriberInterface {
   +
   

   Unrelated change

3. +++ b/core/modules/field/tests/src/Kernel/FieldTypePluginManagerTest.php
   @@ -14,6 +15,15 @@
   +  /**
   +   * Flag to exclude deprecated modules from the tests.
   +   *
   +   * @var bool
   +   */
   +  protected $excludeDeprecated = TRUE;
   
   @@ -119,6 +129,8 @@ protected function enableAllCoreModules() {
   +    $module_list = $this->removeDeprecatedModules($module_list,
   +      $this->excludeDeprecated);
   

   I am not sure why this is class property and then it is only used 1 place?

   But then where it is used it is the default value to removeDeprecatedModules() anyways

4. +++ b/core/modules/field/tests/src/Kernel/LegacyFieldTypePluginManagerTest.php
   @@ -0,0 +1,20 @@
   +   * Flag to exclude deprecated modules from the tests.
   +   *
   +   * @var bool
   +   */
   +  protected $excludeDeprecated = FALSE;
   

   Oh I see now why \Drupal\Tests\field\Kernel\FieldTypePluginManagerTest::$excludeDeprecated is set now to the default.

   So we have this pattern in all the tests that use DeprecatedModulesTestTrait to have the test that will be extended with the Legacy*Test to declare

   protected $excludeDeprecated = TRUE;
   and then the actual Legacy*Test to declare

   protected $excludeDeprecated = FALSE;

   and then in all the tests you have to have the exact same docblock in both test classes

   /**
    * Flag to exclude deprecated modules from the tests.
    *
    * @var bool
    */
   

   It seems like it would easier to have
   protected $excludeDeprecated = TRUE;
   directly on
   DeprecatedModulesTestTrait
   Then half the classes won't need to declare this at all
   and the other half the Legacy*Test classes could have

   /**
    * {@inheritdoc}
    */
    protected $excludeDeprecated = FALSE;
   
   

   Therefor you won't have to have the duplicate docblock on all these classes.

   You could also then remove the $remove parameter from \Drupal\Tests\DeprecatedModulesTestTrait::removeDeprecatedModules()
   and just access it via $this->excludeDeprecated like $this->deprecatedModules is accessed.

   That way the parmeter doesn't have to be documented either which is basically a duplication of the $this->excludeDeprecated field documentation.

Good feedback, thanks! This patch addresses the issued in #97. I also removed the unrelated changes, even if the block_place.module -file change is coding standards issue (and was autofixed by my linter).

@rpsu thanks for the updates

1. +++ b/core/modules/help_topics/tests/src/Functional/HelpTopicsSyntaxTest.php
   @@ -19,6 +20,13 @@
   +  /**
   +   * {@inheritdoc}
   +   */
   +  protected $excludeDeprecated = TRUE;
   

   For all the tests in this patch that do not start with Legacy* we don't actually need this field in the class at all. Since it is on the trait with the same value.

2. +++ b/core/modules/help_topics/tests/src/Functional/HelpTopicsSyntaxTest.php
   @@ -36,6 +44,9 @@ public function testHelpTopics() {
   +    // Exclude deprecated modules from the modules list.
   +    $module_directories = $this->removeDeprecatedModules($module_directories,
   +      $this->excludeDeprecated);
   

   We don't need the 2nd argument now.

   I personally don't think we need the comment either because the method name explains and the docblock with give all the details.

   But I don't feel strongly about this.

   I think most or all of the calls to removeDeprecatedModules() still have 2nd argument.

3. +++ b/core/modules/migrate_drupal/tests/src/Kernel/LegacyStateFileExists.php
   @@ -0,0 +1,23 @@
   +/**
   + * Tests that core modules have a migrate_drupal.yml file as needed.
   + *
   + * Checks that each module that requires a migrate_drupal.yml has the file.
   + * Because more that one migrate_drupal.yml file may have the same entry the
   + * ValidateMigrationStateTest, which validates the file contents, is not able
   + * to determine that all the required files exits.
   

   I generally think we should not be duplicating the documentation of the class the Legacy* test classes are extending.

   One suggestion would be this pattern

   /**
    * Extends HelpTopicsSyntaxTest to test with deprecated modules.
    *
    * @see \Drupal\Tests\DeprecatedModulesTestTrait::removeDeprecatedModules()
   

   I not sure if we can get away with not doing the full namespace in the first line comment. But I couldn't keep it under the limit if I added the full name space.

4. +++ b/core/modules/migrate_drupal/tests/src/Kernel/Plugin/migrate/DestinationCategoryTest.php
   @@ -26,7 +27,7 @@ class DestinationCategoryTest extends MigrateDrupalTestBase {
   -
   +  use DeprecatedModulesTestTrait;
   

   although this doesn't fail our coding standards this remove the empty line between the trait use statements the rest of the class. I think it is more readable and we usually have this empty line.

5. +++ b/core/modules/migrate_drupal_ui/tests/src/Kernel/MigrationLabelExistTest.php
   @@ -14,21 +15,23 @@
   -    // Install all available modules.
   -    $module_handler = $this->container->get('module_handler');
   -    $modules = $this->coreModuleListDataProvider();
   -    $modules_enabled = $module_handler->getModuleList();
   -    $modules_to_enable = array_keys(array_diff_key($modules, $modules_enabled));
   -    $this->enableModules($modules_to_enable);
   +
   +    $this->enableAllModules();
   
   @@ -37,4 +40,19 @@ public function testLabelExist() {
   +  /**
   +   * Enables all modules, except the ones marked as deprecated.
   +   */
   +  public function enableAllModules() {
   +    // Install all available modules.
   +    $module_handler = $this->container->get('module_handler');
   +    $modules = $this->coreModuleListDataProvider();
   +    $modules_enabled = $module_handler->getModuleList();
   +    $modules_to_enable = array_keys(array_diff_key($modules, $modules_enabled));
   +    $modules_to_enable = $this->removeDeprecatedModules($modules_to_enable,
   +      $this->excludeDeprecated);
   +    $this->enableModules($modules_to_enable);
   +
   +  }
   

   Now that we have trait the only change between the code that is moved into the function is the call to removeDeprecatedModules().

   So it seem it would be better to just add that 1 line. Especially because the function name is enableAllModules() but then in the docblock we have explain that actually no it is not all modules 😜.

6. +++ b/core/modules/migrate_drupal_ui/tests/src/Kernel/MigrationLabelExistTest.php
   @@ -14,21 +15,23 @@
   -    // Get all the migrations
   +    // Get all the migrations.
   

   Unrelated change.

7. +++ b/core/tests/Drupal/Tests/DeprecatedModulesTestTrait.php
   @@ -0,0 +1,49 @@
   +
   

   Generally we don't have empty lines to start off functions. There might be a couple of cases of this in the patch.

8. +++ b/core/tests/Drupal/Tests/DeprecatedModulesTestTrait.php
   @@ -0,0 +1,49 @@
   +    if ((bool) $this->excludeDeprecated) {
   

   I don't think we need the cast to bool here. Doesn't php do this automatically for if statements?

   Check some other similar cases like \Drupal\Tests\BrowserHtmlDebugTrait::$htmlOutputEnabled and we don't do this cast.

9. +++ b/core/tests/Drupal/Tests/DeprecatedModulesTestTrait.php
   @@ -0,0 +1,49 @@
   +      // Remove matching item searching matching values.
   +      $modules = array_diff($modules, $this->deprecatedModules);
   +      // Remove matching item searching matching keys.
   +      $modules = array_diff_key($modules, array_flip($this->deprecatedModules));
   

   I am little confused by these comments.

   They seem to just be explaining what the php array functions. I don't think we need that.

   Without reading more of the issue I don't know why we are searching for the module names as both values and keys of the array. I think if this code needs comment that would be better.

   In which cases would be 1 or the other?

10. +++ b/core/modules/block_place/block_place.module
    @@ -3,6 +3,12 @@
    + * @deprecated in drupal:8.8.0 and will be removed from drupal:9.0.0. Using
    + * Layout Builder (available in Drupal 8.7.0) module is recommended, or
    + * alternatively you may use the contrib module Place Blocks.
    

    1 last thing related to #33, #37 and #38

    It seems weird to me that here and elsewhere we are recommending Layout Builder as a replacement.

    "Place Blocks" can place blocks in your theme regions. Layout Builder can't do that(currently). But if I was using "Place Blocks" and someone told be I could use Layout Builder instead if I didn't know what layout builder did I would assume it could at least do what the module I was previously using could do.

    Of course you can leave certain theme regions empty or remove them to get the same effect using Layout Builder but it is more complicated and takes work and planning.

    It seems like the real replacement is either use the contrib project or just turn it off and use the Block UI module.

    Luckily this module only provides a different UX so that if you turn it off all your blocks will still work.

    I think offering Layout Builder as replacement here could do more harm than good. One of the big UX problems we had to solve in Layout Builder is getting the user to understand which parts of the page are controlled by layout builder and which are not.
    see #2935779: Come up with an affordance for what parts of the page are/are not affected by Layout Builder and #2938182: Design intuitive affordances for Layout Builder (for illustrating which parts of the page are editable in a given context)
    Suggesting it as replacement for "Place Blocks" just muddies the waters.

    What if a user on this advice decides to use layout builder and they don't understand which parts of the page Layout builder controls. It could also be more confusing because when you are viewing */layout the themes regions might not be shown because they may be empty. So if the layout is set up with a header and multi-column sections it might look like your theme regions.

    It could be extra confusing because some of the users of "Place Blocks" might already using Layout Builder. This advice might give them the impression that they are is extra feature of Layout Builder they haven't discovered, placing blocks in regions.

@tedbow Regarding #100.10, maybe we should just say "See the change record for a list of alternatives"? We mention lb_everywhere there as well as the contrib replacement module: https://www.drupal.org/node/3081957

1. re #101 @xjm yeah that sounds good.
2. re #100.9
   

   +++ b/core/modules/help_topics/tests/src/Functional/HelpTopicsSyntaxTest.php
   @@ -36,6 +44,9 @@ public function testHelpTopics() {
   +    // Exclude deprecated modules from the modules list.
   +    $module_directories = $this->removeDeprecatedModules($module_directories,
   +      $this->excludeDeprecated);
        \Drupal::service('module_installer')->install(array_keys($module_directories));
   

   I think the only case where the $modules array has the module names in the keys of the array instead of the values when calling \Drupal\Tests\DeprecatedModulesTestTrait::removeDeprecatedModules()

   I think we should fix this before the method is called with something like

   $modules_to_install = array_keys($module_directories);
   

   Then can just use this var when installing the modules via \Drupal::service('module_installer')->install() a few lines later.

   That way \Drupal\Tests\DeprecatedModulesTestTrait::removeDeprecatedModules() can be a 1 liner.

   return $this->excludeDeprecated ? array_diff($modules, $this->deprecatedModules) : $modules;
   

• #100.1 - addressed, field removed.
• #100.2 - addressed, and the (leftover) comment removed. The removeDeprecatedModules signature does have 2nd argument anymore, these should've been removed with #98
• #100.3 - addressed. the few classes where the class name could be mistaken (for an example Drupal\Tests\migrate_drupal\Kernel\d7\LegacyMigrationProcessTest and Drupal\Tests\migrate_drupal\Kernel\d6\LegacyMigrationProcessTest) full class name with namespace is used even if it makes the docblock short description too long to fit in one line.
• #100.4 - unintentional change, but took an opportunity and sort all the use's in the alphabetical order in all the tests included in the patch
• #100.5 - addressed - and now also MigrationLabelExistTest is in align with how all other tests do that.
• #100.6 - reverted. However, since coding standards dictate there should be a full sentence and a dot, how should this kinf of issues be addressed (if at all)?
• #100.7 - I'm not sure what is this? There is no empty line in the beginning of the file core/tests/Drupal/Tests/DeprecatedModulesTestTrait.php?
• #100.8 - addressed, casting removed
• #100.9,#102 - good point, I failed to spot the small change needed in Drupal\Tests\help_topics\Functional\HelpTopicsSyntaxTest::testHelpTopics
• #100.10 - the text updated all round, removed the suggested alternatives and updated warnings and other texts to point to the CR

and thank you @tedbow and @xjm for the good feedback!

1. +++ b/core/modules/block_place/block_place.info.yml
   @@ -1,6 +1,6 @@
   -description: 'Allow administrators to place blocks from any Drupal page. This module is deprecated in Drupal 8.8.0 and will be removed in Drupal 9.0.0. Using Layout Builder (available in Drupal 8.7.0) module is recommended, or alternatively you may use the contrib module Place Blocks. See https://www.drupal.org/node/3081957.'
   +description: 'Allow administrators to place blocks from any Drupal page. This module is deprecated in Drupal 8.8.0 and will be removed in Drupal 9.0.0, see the change record for a list of alternatives. See https://www.drupal.org/node/3081957.'
   

   One small point for the deprecation message changes. The message should be formatted like:

   Allow administrators to place blocks from any Drupal page. This module is deprecated in Drupal 8.8.0 and will be removed in Drupal 9.0.0. See the change record for a list of alternatives.
   

   (So that it's a separate sentence.)

2. +++ b/core/modules/block_place/block_place.module
   @@ -21,9 +20,8 @@ function block_place_help($route_name, RouteMatchInterface $route_match) {
   -      $output .= '<p><strong>' . t('Place Blocks (Core, Experimental) is deprecated in Drupal 8.8.0 and will be removed in Drupal 9.0.0. Using Layout Builder (available in Drupal 8.7.0) module is recommended, or alternatively you may use the contrib module <a href=":contrib-link">Place Blocks</a>. See <a href=":change-record">the change record</a>.', [
   +      $output .= '<p><strong>' . t('Place Blocks (Core, Experimental) is deprecated in Drupal 8.8.0 and will be removed in Drupal 9.0.0, see the change record for a list of alternatives. See <a href=":change-record">the change record</a>.', [
   

   For this sentence, since it's in the user interface, we don't need to repeat it. I think it can just say:

   'Place Blocks (Core, Experimental) is deprecated in Drupal 8.8.0 and will be removed in Drupal 9.0.0. <a href=":change-record">See the change record for a list of alternatives</a>.'
   

   (That's also more accessible so that the link text makes sense out of context.)

I'll actually just incorporate those changes quickly to save time.

This looks great!

Two remaining micro nits:

1. +++ b/core/modules/block_place/block_place.install
   @@ -0,0 +1,34 @@
   +      'description' => t('Place Blocks (Core, Experimental) is deprecated in Drupal 8.8.0 and  will be removed in Drupal 9.0.0. See <a href=":change-record">See the change record for a list of alternatives</a>.', [
   ...
   +  \Drupal::messenger()->addWarning(t('Place Blocks (Core, Experimental) is deprecated in Drupal 8.8.0 and  will be removed in Drupal 9.0.0. See the change record for a list of alternatives. See <a href=":change-record">the change record</a>.', [
   

   and will double space

2. +++ b/core/modules/block_place/src/Plugin/DisplayVariant/PlaceBlockPageVariant.php
   @@ -56,6 +61,12 @@ class PlaceBlockPageVariant extends BlockPageVariant {
   +    @trigger_error('The ' . __NAMESPACE__ . '\PlaceBlockPageVariant is
   +     deprecated in drupal:8.8.0 and will be removed from drupal:9.0.0. See the
   +     change record for a list of alternatives. See
   +     https://www.drupal.org/node/3081957.', E_USER_DEPRECATED);
   

   This is indented weirdly with one space (and weird that the string is broken up like this at all? Does that introduce extra spaces?)

• #108.1 - whitespace removed
• #108.2 - @trigger_error message's newlines removed as it does introduce extra spaces (due to indenting).

+++ b/core/modules/block_place/block_place.install
@@ -0,0 +1,34 @@
+      'description' => t('Place Blocks (Core, Experimental) is deprecated in Drupal 8.8.0 and  will be removed in Drupal 9.0.0. See <a href=":change-record">See the change record for a list of alternatives</a>.', [

Another double-spaced "and will" here too.

re #103
@rpsu thanks for the continue updates!

1. re #100.6
   Generally I don't think we are supposed to address coding standard on lines that we are not updating.

   This makes it easier to review patches but also makes it less likely to conflict with other patches that might affect those lines(for a non coding standards fix. Also because of the way patches work if you the lines surrounding it changed in a commit that patch would no longer apply.

   Generally existing coding standards are addressed in 1 patch for the whole rule across the code base. See #2719663: Fix 'Drupal.Commenting.InlineComment.InvalidEndChar' coding standard which does fix this issue, and actually needs work.

   This 1 line change is not big deal but often there are more standards problems in the same so it because worse for patches.

2. #100.7

   Weird.

   There still some cases of this
   \Drupal\block_place\Plugin\DisplayVariant\PlaceBlockPageVariant::__construct()

   Also DeprecatedModulesTestTrait

   protected $excludeDeprecated = TRUE;
   

3. re #107 @xjm these changes also look good.
4. Unrelated to recent changes.

   I am not sure why certain tests where selected to use this new trait.

   But I checked removing the use of the trait on \Drupal\Tests\migrate_drupal\Kernel\d7\MigrationProcessTest which of course removes it from \Drupal\Tests\migrate_drupal\Kernel\d7\LegacyMigrationProcessTest

   Both tests pass without the trait.

   Also true for \Drupal\Tests\migrate_drupal\Kernel\d7\MigrateDrupal7AuditIdsTest

   I am probably missing something but I thought I would point that out just incase

I didn't see #109 but it look like #111.2 still is not fixed

• #110 - fixed
• #111.1 - okay, thanks for the info!
• #111.2 / #100.7 - looking at the patch itself (the file, without Dreditor) this does not show. I suppose the Dreditor trips to something and displays the line numbers off by one?
• #112.4 - The tests that are using the trait are the ones that were failing, because the tests (blindly) enable apparently every core module. Most of them I was able to track, one of them I could not even spot where all the core modules were enabled.

In #89 I pointed out that the deprecation in block_place.module was incorrectly referring to the PageVariant.
#90, instead of changing the text, removed the entire trigger_error().

This made some of the test changes obsolete (like FieldTypePluginManagerTest, I haven't tried them all).

The trigger_error should be restored to the .module

ok just commenting out the 1 line in the trait to see which tests fail.

changing drupalci.yml because we only changed kernel and functional tests

also a patch with .module file trigger_error back to see the difference in fails

1. re #111
   

   looking at the patch itself (the file, without Dreditor) this does not show. I suppose the Dreditor trips to something and displays the line numbers off by one?

   I was looking at the code with the patch applied. with #113 I still see the 2 extra lines mentioned

   Yes these extra lines are hard to see with dreditor. Can't wait till gitlab review integration!

   Fixed

2. Adding back trigger_error in .module file
3. Will have to see results of 3062281-116-test-for-fails-with-module-trigger.patch but if there any of the updated or new tests that pass with that patch then either we don't need them or will need to figure out what is going on

re #116

+++ b/core/tests/Drupal/Tests/DeprecatedModulesTestTrait.php
@@ -35,7 +35,7 @@ trait DeprecatedModulesTestTrait {
-    return $this->excludeDeprecated ? array_diff($modules, $this->deprecatedModules) : $modules;
+    //return $this->excludeDeprecated ? array_diff($modules, $this->deprecatedModules) : $modules;

whoops i realise now I should have just changed this to

return $modules;

Oh well. Running locally

another try at the idea in #116

1. ok I see why \Drupal\Tests\migrate_drupal\Kernel\d7\ValidateMigrationStateTest failed in #117

   \Drupal\Tests\migrate_drupal\Traits\ValidateMigrationStateTestTrait::testMigrationState calls \Drupal\Tests\migrate_drupal\Traits\ValidateMigrationStateTestTrait::enableAllModules so the deprecate modules aren't filtered out.

   But it seems like we can just add an override of \Drupal\KernelTests\KernelTestBase::enableModules() which called removeDeprecatedModules()to DeprecatedModulesTestTrait. That the only change to any of the kernel tests would be to use the trait. You won't need any other changes.

   The only other test that needs to change that is not a kernel test is \Drupal\Tests\help_topics\Functional\HelpTopicsSyntaxTest in that case we can just the current change in the patch should work. It calls \Drupal::service('module_installer')->install($modules_to_install); to install modules so that would be difficult to override.

2. Update......

   Ok it seems like there is another problem some of the tests do

   self::$modules = array_keys($this->coreModuleListDataProvider()); 
   

   in setUp

   So then in setUp ultimately \Drupal\KernelTests\KernelTestBase::getModulesToEnable() is going to be called. This is static so we can't override it our trait because we need to access which we can't in a static function ☹️

   So for those classes when have to add back the logic to call

   self::$modules = $this->removeDeprecatedModules(self::$modules);
   

   in setup.

1. I just missed a test class for #124.2

   🤞tests should pass now.

2. The only things I want to point out about the current patch is \Drupal\Tests\DeprecatedModulesTestTrait::enableModules() is on trait that is used by \Drupal\Tests\help_topics\Functional\HelpTopicsSyntaxTest which is not a kernel test. So it won't have a parent::enableModules() if someone tried to call that later. But it would fatal error so I think that is fine. the docblock mentions it for kernel tests.

+++ b/core/modules/block_place/src/Plugin/DisplayVariant/PlaceBlockPageVariant.php
@@ -56,6 +61,8 @@ class PlaceBlockPageVariant extends BlockPageVariant {
+    @trigger_error('The ' . __NAMESPACE__ . '\PlaceBlockPageVariant is deprecated in drupal:8.8.0 and will be removed from drupal:9.0.0. See the change record for a list of alternatives. See https://www.drupal.org/node/3081957.', E_USER_DEPRECATED);

instead of __NAMESPACE and then typing out the class name, use self::class

for 126.2, I think it's fine as is. Traits and parent:: are weird anyway, and this doesn't make that worse.

I chatted with @tim.plunkett about this more

search core with regex

@trigger_error\(.*__NAMESPACE there are over 100 results

@trigger_error\(.*self:: there is only 1 existing result

So it makes more sense to leave it the way it is.

Yeah I think those 174 instances are wrong, but let's be consistent here.

Thanks @tedbow!

Patch didn't apply with git to 9.0.x, but only context so I fixed that locally.

Added a release notes snipped and tagging.

Committed/pushed to 9.0.x/8.9.x and 8.8.x, thanks!

Great work, thank you!

Published the CR.

@xjm is the correct release 8.8.0-alpha2, or -beta1? CR states 8.8.0-alpha2 (which I wrote there a few weeks ago).

Adding a link to the CR to the end of the release note.

@rpsu, good catch on the release version for the CR. We generally don't issue an alpha2 unless there's a critical hotfix needed. I'll change it to say beta1.

Ah, I see you fixed it already. :)