Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
I feel like this is probably already covered somewhere, but looking through the critical bugs I didn't see a title that rang a bell...
If you create a views block then place it into a region, then delete the block through Views UI, every page on your site gets this:
Fatal error: Call to a member function setDisplay() on a non-object in /Users/webchick/Sites/8.x/core/modules/views/lib/Drupal/views/Plugin/block/block/ViewsBlock.php on line 52
This is because although the views.view.foo.yml file is removed from the active store, block.block.bartik.views_foo.yml is not. You can remove it manually as a workaround, but that's not very nice.
Comment | File | Size | Author |
---|---|---|---|
#15 | vdc-1898804-15.patch | 10.79 KB | tim.plunkett |
#15 | interdiff.txt | 2.07 KB | tim.plunkett |
#10 | drupal-1898804-10.patch | 10.51 KB | dawehner |
#10 | interdiff.txt | 9.57 KB | dawehner |
#8 | interdiff.txt | 1.5 KB | dawehner |
Comments
Comment #1
dawehnerLet's start with a simple approach, though we need tests and an actual fix :)
Comment #2
dawehnerThis time it works, working on tests right now.
Comment #3
tim.plunkettThis would be a great place for the new config entity query!
Comment #4
dawehnerI totally agree, there are also multiple places in block.module which could use that.
This time with the full test code.
Comment #6
tim.plunkettFixing indentation isn't in scope :)
We don't use $block_id here
This seems like it would delete all placed blocks of all displays of all views if one block display was removed
This is a great idea!
Reacts
Doesn't need the blank line
I'd recommend just using drupalPlaceBlock() in the test, and not providing a default config file.
We should add explicit coverage for:
Adding two instances of the same display, and making sure they are both removed
A view with two block displays, adding one placed block instance of each, removing one display and making sure the other is still there
A second view with a block display with an instance, that shouldn't be affected by the deletion of a display on another view
Comment #7
dawehnerThanks for suggestion all this possible combinations.
This piece of indentation is brutal.
Comment #8
dawehnerNever fix something right before uploading the patch without rerunning the test.
Comment #9
tim.plunkettIt doesn't really matter, but I'd probably rewrite this as
These should be moved out of the config directory and into the test_views directory.
I think that setUp needs to come first, or the modules won't be enabled?
Checks
These are similar enough, is it worth writing a helper method?
Here, and elsewhere: assertFalse and assertTrue cast the value to (bool) for you.
Missing
hidden = TRUE
Comment #10
dawehnerOne detail which maybe interests you: I had to put that into the config directory, as enabling the block module causes the block entities to be imported but break as the view didn't existed before.
That's also the reason for
There we go.
Comment #11
dawehner.
Comment #13
dawehner#10: drupal-1898804-10.patch queued for re-testing.
Comment #15
tim.plunkettIf you use assertTrue with no message, it tries to print out the whole value, which in this case was a block config entity, causing the "Nesting level too deep - recursive dependency?" fatal.
The test coverage looks good and the fix makes sense.
Comment #16
dawehnerHey you suggested that assertFalse would already cast to bool :)
Comment #17
catchThis looks like it should be an API function provided by block module, i.e. "delete all instances of this block". I can see lots of modules having to copy and paste this code.
Also I'm wondering a bit whether the responsibility here should entirely fall on the Views display plugin, or if it does, it feels like there should be docs in block module pointing out that if you remove a block type you also need to delete the instances yourself.
What happens with aggregator feeds and instances of those blocks? Or menu blocks?
Comment #18
tim.plunkettWhen modules invoke hooks for delete (like hook_menu_delete()), block.module implements the hook.
Other cases, like Views or Aggregator's aggregator_save_category() or aggregator_save_category(), have to handle it themselves.
I think it might be worth coming up with a standard pattern for this, but I'm pretty sure it should be a module's job to handle this.
Comment #19
tim.plunkettBasically, derivative discovery is passive, and defined internally by each module, so it should be their job to clean it up. If we do anything, we should put docs on DerivativeDiscoveryDecorator.
Comment #20
tim.plunkettIt seems the generic issue for this is #1886462: Determine how to clean up plugin instances when the derivative definitions change, but this fixes the bug in a manner consistent with other derivatives.
Comment #21
catchOK thanks for the link to the generic issue, following that one now. Committed/pushed this to 8.x.
Comment #22.0
(not verified) CreditAttribution: commentedx