Problem/Motivation

I have a module that decorates \Drupal\Core\Entity\HtmlEntityFormController, constructor typehinted by its abstract base class \Drupal\Core\Controller\FormController. (Note: apart from this abstract base class, there is NO interface.)

When updating to 8.8.5, the module breaks with:
TypeError: Argument 3 passed to Drupal\ogmedia_group_stub\GroupStubEntityFormControllerDecorator::__construct() must be an instance of Drupal\Core\Controller\FormController, instance of Drupal\layout_builder\Controller\LayoutBuilderHtmlEntityFormController given, called in /var/www/html/web/core/lib/Drupal/Component/DependencyInjection/Container.php on line 281

This comes from the fact, that the newly introduced LayoutBuilderHtmlEntityFormController decorates that controller too, in its constructor typehints to FormController, bot does NOT extend that base class.

Proposed resolution

Make LayoutBuilderHtmlEntityFormController extend \Drupal\Core\Controller\FormController

In a followup consider adding an interface.

Remaining tasks

Do it.

User interface changes

None.

API changes

None.

Data model changes

None.

Release notes snippet

n/a

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

geek-merlin created an issue. See original summary.

geek-merlin’s picture

Patch flying in that should fix it.

geek-merlin’s picture

Where this originated.

geek-merlin’s picture

Patch fixes the bug for me.

tim.plunkett’s picture

Status: Needs review » Needs work
Issue tags: +Blocks-Layouts

Good catch, I messed that up due to there not being an interface.
Can you file a follow-up to add an interface? I feel like that would be ideal when decoration is called for.

Only nits:

  1. +++ b/core/modules/layout_builder/src/Controller/LayoutBuilderHtmlEntityFormController.php
    @@ -11,7 +11,7 @@
    +class LayoutBuilderHtmlEntityFormController extends FormController{
    

    Missing space before {

  2. +++ b/core/modules/layout_builder/src/Controller/LayoutBuilderHtmlEntityFormController.php
    @@ -56,4 +56,18 @@ public function getContentResult(Request $request, RouteMatchInterface $route_ma
    +   * @inheritDoc
    ...
    +   * @inheritDoc
    

    {@inheritdoc}

Neslee Canil Pinto’s picture

Status: Needs work » Needs review
FileSize
1.16 KB
1.07 KB

Made requested changes from #5.

geek-merlin’s picture

Status: Needs review » Reviewed & tested by the community

All nits from #5 have been addressed, setting RTBC.

geek-merlin’s picture

Added followup.

tim.plunkett’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs tests, +Needs followup
+++ b/core/modules/layout_builder/src/Controller/LayoutBuilderHtmlEntityFormController.php
@@ -56,4 +56,18 @@ public function getContentResult(Request $request, RouteMatchInterface $route_ma
+   * {@inheritDoc}
...
+   * {@inheritDoc}

Lowercase d


This should be testable from something like layout_builder_test module. If it attempts to decorate LayoutBuilderHtmlEntityFormController, it should fail with the current code and pass with the patch.

And it still needs the follow-up from #5.

Version: 8.9.x-dev » 9.1.x-dev

Drupal 8.9.0-beta1 was released on March 20, 2020. 8.9.x is the final, long-term support (LTS) minor release of Drupal 8, which means new developments and disruptive changes should now be targeted against the 9.1.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

Deepak Goyal’s picture

Status: Needs work » Needs review
FileSize
1.16 KB
1.04 KB

Hi @tim.plunkett I have updated the patch please review.

tim.plunkett’s picture

Status: Needs review » Needs work

Thanks for the capitalization fix. Still needs tests and a follow-up.

danflanagan8’s picture

Here's a patch with a FAIL test followed by a patch that includes the code from #11 tested against the fail patch.

tim.plunkett’s picture

Issue tags: -Needs tests

Test looks great, just some nits from copy/paste:

  1. +++ b/core/modules/layout_builder/tests/src/Functional/LayoutBuilderTest.php
    @@ -447,6 +447,24 @@ public function testLayoutBuilderUi() {
    +    $page = $this->getSession()->getPage();
    

    Doesn't look like this is used

  2. +++ b/core/modules/layout_builder/tests/src/Functional/LayoutBuilderTest.php
    @@ -447,6 +447,24 @@ public function testLayoutBuilderUi() {
    +    $field_ui_prefix = 'admin/structure/types/manage/bundle_with_section_field';
    +    $this->drupalGet("$field_ui_prefix/display/default");
    

    Don't need a variable for one usage.

From #5

Can you file a follow-up to add an interface? I feel like that would be ideal when decoration is called for.

danflanagan8’s picture

Updated tests in new patch, @tim.plunkett.

Regarding the follow-up issue, it looks like @geek-merlin created one and set it as a related issue: #3129641: Add missing FormControllerInterface

Or were you wanting me to put a @todo note on FormController or something like that?

tim.plunkett’s picture

Status: Needs work » Reviewed & tested by the community
Issue tags: -Needs followup

I cross-posted with that in #4, missed it this whole time. Thanks!

Side-note: are you manually queueing those tests? If you set the issue to Needs Review when posting patches, you don't have to do that!

danflanagan8’s picture

Thanks, @tim.plunkett. Gosh, I had no idea about "Needs Review" queueing a test. I've been picking "Needs Review" after I know the tests pass (or fail if that's what I'm trying for).

I've been trying to get more into helping out with core, as you can probably tell. Thanks for all the help going back and forth with me lately.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 15: 3126746--15.patch, failed testing. View results

larowlan’s picture

saving issue credit

larowlan’s picture

Title: LayoutBuilderHtmlEntityFormController breaks decoration » [backport] LayoutBuilderHtmlEntityFormController breaks decoration
Version: 9.1.x-dev » 9.0.x-dev
Status: Needs work » Reviewed & tested by the community

Committed 6ffbfe2 and pushed to 9.1.x. Thanks!

Flagging for possible backport

larowlan’s picture

Issue tags: +Bug Smash Initiative

  • larowlan committed 6ffbfe2 on 9.1.x
    Issue #3126746 by danflanagan8, Deepak Goyal, Neslee Canil Pinto, geek-...
alexpott’s picture

+1 for a backport.

  • larowlan committed bd3795d on 9.0.x
    Issue #3126746 by danflanagan8, Deepak Goyal, Neslee Canil Pinto, geek-...
larowlan’s picture

Title: [backport] LayoutBuilderHtmlEntityFormController breaks decoration » LayoutBuilderHtmlEntityFormController breaks decoration
Status: Reviewed & tested by the community » Fixed

Backported to 9.0.x

Status: Fixed » Closed (fixed)

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