Problem/Motivation

After sprinting fixing bugs in #1535868: Convert all blocks into plugins we (tim.plunkett, sdboyer and I) agreed that plugin.core.block was a horrible CMI prefix and that it really should just be block.

Proposed resolution

Rename and search/replace all instances of plugin.core.block -> block

Remaining tasks

None

User interface changes

None

API changes

None

Files: 
CommentFileSizeAuthor
#16 block-1839904-15.patch7.98 KBtim.plunkett
PASSED: [[SimpleTest]]: [MySQL] 50,658 pass(es). View
#13 block-1839904-13.patch7.11 KBtim.plunkett
FAILED: [[SimpleTest]]: [MySQL] 50,662 pass(es), 3 fail(s), and 0 exception(s). View
#12 block-1839904-12.patch7.99 KBtim.plunkett
PASSED: [[SimpleTest]]: [MySQL] 50,778 pass(es). View
#1 drupal-rename_plugin_core_block-1839904-1-do-not-test.patch33.9 KBdisasm

Comments

disasm’s picture

Attached is a patch that renames all instances of plugin.core.block to block in all files. This can't be tested yet because it is not implemented in core yet. It's in patch #1535868: Convert all blocks into plugins.

Below are the commands to regenerate this patch after the other patch lands:

find ./ -type f|grep -v '.git'|xargs sed -i 's/plugin.core.block/block/g'
find ./ -type f|grep -v '.git'|xargs perl-rename 's/plugin.core.block/block/'

xjm’s picture

Yes please.

Dave Reid’s picture

How would this work if block module wanted to store extra settings? Wouldn't a config in block.settings be automatically loaded? I think having two levels for the plugin storage still might be good.

xjm’s picture

Views does views.view.* and views.settings.

xjm’s picture

Status: Active » Postponed

Marking this postponed on the main issue.

xjm’s picture

Status: Postponed » Active
xjm’s picture

xjm’s picture

sun’s picture

Status: Postponed » Active

Quoting myself from #1826602-87: Allow all configuration entities to be enabled/disabled:

[The problem of this issue here] is much rather that the new config files of the new block system are not namespaced + owned + supplied by each module, and instead, collected by a single service. Therefore, the service has to manually check whether the configuration is actually valid, and it also has to maintain the configuration on behalf of the actual modules.

Instead of plugin.core.block.ID config names, the block system should actually use $module.plugin.core.block.ID.

I wanted to raise this concern in the block plugin conversion issue already, but didn't want to block that monster patch on it.

On the question of block.plugin.$module.ID vs. $module.plugin.block.block.ID, the actual questions need to be:

1) Who owns the config object? Block module or the $module? ("owns" means responsibility. Think of maintenance, update.php.)

2) Also: Are block plugins applicable to Block module only? Or can they be re-used by other services/modules? (What about SuperPanels³-NG 8.x-2.x in contrib? And also, wasn't the plan to blockify the entire system in the end...?)

tim.plunkett’s picture

Status: Active » Postponed

This was postponed on the ConfigEntity issue for a reason, because it will mean we should follow the lead of every other ConfigEntity and make it block.block.

xjm’s picture

Issue tags: +Plugin system, +Block plugins
tim.plunkett’s picture

Status: Postponed » Needs review
FileSize
7.99 KB
PASSED: [[SimpleTest]]: [MySQL] 50,778 pass(es). View
tim.plunkett’s picture

FileSize
7.11 KB
FAILED: [[SimpleTest]]: [MySQL] 50,662 pass(es), 3 fail(s), and 0 exception(s). View
tim.plunkett’s picture

Title: Rename plugin.core.block CMI prefix to block » Rename plugin.core.block CMI prefix to block.block
EclipseGc’s picture

Status: Needs review » Needs work

minimal profile's blocks need converting as well.

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
7.98 KB
PASSED: [[SimpleTest]]: [MySQL] 50,658 pass(es). View
EclipseGc’s picture

Status: Needs review » Reviewed & tested by the community

This looks completely sane if testbot is happy.

Eclipse

sun’s picture

Hm. What about #9 though?

My larger review in #1871696-43: Convert block instances to configuration entities to resolve architectural issues contained some more details:

Also, the theme name as prefix was very unexpected — given that these config objects are the canonical representations of plugin instances, and given that every plugin instance is of a certain type, which in turn is provided by a certain module/extension, I would have expected config object names of:

block.block.$extension.$plugin_base_id.$machine_name

That would inherently solve a couple of problems that are apparent in the current code:

  • A module gets uninstalled?

    No problem: config()->listAll("block.block.$module."); Process the limited subset. Done.

  • A plugin ID ceases to exist or a derivative is deleted?

    No problem: config()->listAll("block.block.$module.$plugin_base_id"); Process the limited subset. Done.

The existing hook implementation for deleted menu blocks would be vastly simplified. And perhaps I'm wrong, but I think it is safe to expect that we'll actually have a lot of derivative plugin implementations + instances in D8, so architecturally + storage-wise, we should be prepared for that.

If I get it right, then the theme name was chosen as prefix, since we need to load all block instances for a theme. But frankly, I'd much rather solve that use-case through a cache (or even state?) of block entity IDs per theme. The storage/identifiers should be optimized for handling CRUD operations.

Alternatively/additionally, I could also offer to extend listAll(), so that it is possible to use wildcards within config names to find; e.g., listAll("block.block.*.*.$theme."), and we'd use config object names of

block.block.$extension.$plugin_base_id.$theme.$machine_name

(which would limit the machine_name length to 250 - 65 - 65 - 65 - 12 == 43 characters, but that should be sufficient).

Anyway, point taken. I think the config object names in this patch require some more architectural design considerations.

tim.plunkett’s picture

None of that is this issue's problem to solve. This merely brings Block inline with everything else. There are other issues for that.

webchick’s picture

Status: Reviewed & tested by the community » Fixed

Committed and pushed to 8.x. Thanks! Don't think this needs a change notice, since it's D8 only.

Would you be able to cross-link those issues for where follow-up discussion is happening?

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