Problem/Motivation

As found in #2309247: Views do not depend on modules providing their displays several views have block displays. The block displays were used to be provided by block module so a mechanism was added to bubble up that dependency to the view level. However, since then the block display was moved to views in #2315333: Move block plugin code out of block.module. The views themselves have not been updated.

Proposed resolution

Update the views to not depend on block module anymore. Now the views only depend on the modules they are contained with so no need to move them around or set further dependency on the modules.

Remaining tasks

Review. Commit.

User interface changes

None.

API changes

None.

Files: 
CommentFileSizeAuthor
#6 2309715-block-displays-are-now-in-views-6.patch8.02 KBGábor Hojtsy
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,111 pass(es). View
#6 interdiff.txt1.71 KBGábor Hojtsy
#4 2309715-block-displays-are-now-in-views.patch6.31 KBGábor Hojtsy
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,120 pass(es). View

Comments

Gábor Hojtsy’s picture

Status: Postponed » Active

#2309247: Views do not depend on modules providing their displays landed. Moshe posted these notes there:

If a module happens to ship with a View that takes advantage of block module, it does not mean that the module *depends* on block. Moving things to standard profile is a convenience that Contrib doesn't have. I'd rather not see that workaround here - or perhaps I am misunderstanding.

It is not a *dependency*. It is a *suggestion* or a recommendation. Composer uses the term suggestion for this sort of relationship - https://getcomposer.org/doc/04-schema.md#suggest.

The problem with being strict here is that up and coming modules have no chance of growing. Lets say I spot a cool new module that has almost no usage. They have a great API and it only takes me a few lines in my module to support it. If my module has to *depend* on the new module, then I create a new hassle and decision point for anyone updating to latest version. I might even choose not to support the new module so as to keep my dependency list low.

Looks like we don't have an agreement on this. As the current config stands, it may include fragments that are unknown to the system. One of the 3 remaining beta blockers is exactly about this "random fragment of thing in config" problem at #2224761: Add a generic way to add third party configuration on configuration entities and implement for field configuration.

At least for translations, its problematic that random fragments may be included and we have no way to figure out the config schemas for them. So none of the translatable strings in them will ever show up on localize.drupal.org for example.

I think its generally a problem anyway that modules would need to deal with configuration that may contain things that are structured and owned by an "unknown" thing.

tim.plunkett’s picture

Gábor Hojtsy’s picture

Status: Active » Postponed
Gábor Hojtsy’s picture

Title: Node, comment and user modules have views that also depend on block, move to standard profile » Several views still say they depend on block module but not anymore
Status: Postponed » Needs review
FileSize
6.31 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,120 pass(es). View

#2315333: Move block plugin code out of block.module got committed. This is a much simpler issue now. The block dependency is not there anymore because the block display plugin is provided by views.

Gábor Hojtsy’s picture

Issue summary: View changes
Gábor Hojtsy’s picture

Issue tags: +Configuration schema
FileSize
1.71 KB
8.02 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,111 pass(es). View

Should also move the block display config schema. Not sure why that was not done in #2315333: Move block plugin code out of block.module. No changes to the schema, just moving.

tim.plunkett’s picture

Status: Needs review » Reviewed & tested by the community

The schema move was just an oversight, thanks for fixing that here.

webchick’s picture

Status: Reviewed & tested by the community » Fixed

Nice clean-up.

Committed and pushed to 8.x. Thanks!

  • webchick committed 37709d6 on 8.0.x
    Issue #2309715 by Gábor Hojtsy: Fixed Several views still say they...
jhodgdon’s picture

Status: Fixed » Needs work

On #2323899-15: Provided default Node views need language filtering, gauravkhambhala reports that when exporting the Archive view (for instance) the block section still has "provider: block" in the config. So is this really fixed?

dawehner’s picture

Just to be clear, there is still code in views which relies on the block entity existance. So dropping that was kinda a bad idea.

jhodgdon’s picture

Status: Needs work » Fixed

The person who reported #10 says it is no longer valid. Not sure about dawehner's point; setting back to "Fixed" for #10 though.

Gábor Hojtsy’s picture

@jhodgdon: yeah I found it if I exported a new view, the dependencies were correct, but if I exported an existing view that was installed with the incorrect dependencies, those would be kept. I think (not all) parts of the view are recreated through their plugins again.

@dawehner: does that mean views module now requires block module?

effulgentsia’s picture

Re #11: which code? When I search for "Drupal\block" within core/modules/views, nothing turns up. But maybe the dependency is there and just not explicitly included like that?

dawehner’s picture

@Gábor Hojtsy

Yes it does technically, see \Drupal\views\Plugin\views\display\Block::remove()(which will be resolved in https://www.drupal.org/node/2031859) and all the test code inside block module referring to the views block display.

Gábor Hojtsy’s picture

Woah, does this blow up when block module is not enabled or just silently gets you an empty array? Answer from the code seems to be an empty array:

    $plugin_id = 'views_block:' . $this->view->storage->id() . '-' . $this->display['id'];
    foreach (entity_load_multiple_by_properties('block', array('plugin' => $plugin_id)) as $block) {
      $block->delete();
    }

So this will return an empty array and therefore not delete blocks when you remove a block display IF block module is not enabled, but is that a dependency? Sounds like the right thing happens for the situation at hand. Not sure what test code in block module are you referring to?

dawehner’s picture

All the integration tests between \Drupal\Core\Block and \Drupal\views still exists inside the block module.

This fatals:

$ drush php
[1] self> entity_load_multiple_by_properties('aggregator_item', ['iid' => 0]);
The "aggregator_item" entity type does not exist.                                                            [error]

Note, the remove() call could be triggered during a config import, when a view is removed.

So this will return an empty array and therefore not delete blocks when you remove a block display IF block module is not enabled, but is that a dependency? Sounds like the right thing happens for the situation at hand. Not sure what test code in block module are you referring to?

I have never said it is an actual dependency, but currently it is a technical one, if we are honest. In other words please help on #2031859: Invoke an event[s] when a plugin ID disappears or we will have to make views module be dependent on block module.

Status: Fixed » Closed (fixed)

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