Remove Place Blocks module

I have tried to remove block_place module, please review.

Patch no longer applied. Created a reroll.

@rpsu,

Briefly discussed with @borisson_ and @wimleers. They suggested keeping that trait for now since it's possible that some modules will be marked deprecated in Drupal 9. If not, we can still remove the trait later.

Agree with that too, we'll eventually deprecate modules again.

I confirmed that there is no mention of block_place left after applying the patch.

But I think we can include #3084471: Remove/fix legacy tests after Place Blocks -module removal here as well and unmark these tests?

Agreed, closed #3084471: Remove/fix legacy tests after Place Blocks -module removal as duplicate. @berdir: I don't think we would unmark them, we would remove the legacy ones, no? At least they would be doing the same test twice once we remove block_place from the trait's list.

Depends on the test, but my understanding is that some of these tests are only legacy because they can't avoid touching the block_place module and triggering that message? Typically because they just loop overall modules and install them for things. So we still need their test coverage, but now they shouldn't trigger any deprecation messages anymore?

Bumping to critical since this has to happen before beta (or we'd have to update all the deprecation messages).

With the legacy tests, we already have modules we're about to deprecate in Drupal 9 for Drupal 10, so I'm not sure we can actually unmark everything. Although we could unmark and remark maybe to see what's really necessary? See #3111645: Uninstall entity_reference module and prevent it being enabled again, remove deprecated code for an example, but that shouldn't requite @legacy anywhere since there's not really any PHP code left to deprecate.

entity_reference does, as far as I see, have any code left with @trigger_error()'s in it, so it shouldn't actually trigger anything. The trait is now an empty list of modules, it already doesn't do anything now, so we can definitely remove the usages.

I realized that all usages of the trait come with a subclass that tests not only the legacy modules but all the modules including legacy again, so I definitely think we should clean that up and only if really necessary add it back. (I didn't know that in #8). Not a huge deal for the kernel tests, but HelpTopicsSyntaxTest is no joke.

I never understood the "aggressive" @trigger_error() approach in block_place, as we didn't actually deprecate the API's, just move them out of core, and a single hook_requirements() just like we did now in entity_reference.module would have been enough IMHO :)

Not sure, maybe in a follow-up that's not a beta blocker?

I don't think entity_reference will need this, as we can just remove it in D10, but there might be other modules, although I'll try to push for a simpler approach. To be perfectly clear, I do not mean to say anything negative about the work you did, you simply did what was required to get it committed and prepared for its removal from core.

@catch suggested to just remove the trait and also suggested to change the deprecation approacg in block_place to be less "aggressive", meaning no @trigger_error() in the module file, similar to my change in #3062302: Properly deprecate the entity reference module. (In a follow-up)

There is a single use of it currently in contrib, in a migrate module.

We'll also need a change record for the removal of that trait.

But this trait could be useful in 9.x release cycle, other then that needs CR

One class still had the trait.

And now also removed the call to that method..

I think we need the follow-up to remove the .module trigger_error() from place blocks in 8.8 and 8.9 before we can commit this, but looks good otherwise.

At first sight this issue removing deprecated modules trait looks odd but actually I think if we do need to implement something like that in D9 then we should do it better.

One thought is that essentially we're doing the thing that causes #2917600: update_fix_compatibility() puts sites into unrecoverable state so it would be great to have that fixed.

Here's the 8.8/8.9 issue for Place Blocks: #3116399: [D8 only] Place blocks .module file should not trigger a deprecation

Looks like only 3 contrib projects are going to be affected - not too bad... http://grep.xnddx.ru/search?text=-%20block_place&filename=

Is someone in contact with the https://www.drupal.org/project/block_place maintainers to replace the content of their repo with this module once (or before) this patch goes in?

EDIT: Yep, looks like this was taken care of in #3062281: Deprecate block_place module for removal in Drupal 9.

Committed 262b7fc and pushed to 9.0.x. Thanks!

I'm not sure we wanted to remove Drupal\Tests\DeprecatedModulesTestTrait. I was using it to filter out deprecated modules from my tests since 8.6 or 8.7 and prepare my module for 9.x compatibility. But now with its removal, my tests fall over miserably. Could/should probably re-add it.

See CR https://www.drupal.org/node/3116386.

My point isn't so much that there aren't any deprecated modules in D9, nor that a new way to discover deprecated modules will be available. My point is that a non internal test layer was removed without first deprecating it.

However, I'd propose that additional modules in D9 will also be deprecated. And a test trait like what was removed would still be useful. But that's beside the point, right?

Anyone able to update the IS with a release notes snippet, briefly summarising the effects of this change?

Oh, IS means issue summary. The release notes snippet is part of the issue summary template and should be included for issues that have a potentially disruptive change.

Updated with a correct release note, and polished this in the 9.0.0-alpha2 release notes.