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
Comment | File | Size | Author |
---|---|---|---|
#15 | interdiff_13-15.txt | 1.13 KB | danflanagan8 |
#15 | 3126746--15.patch | 5.66 KB | danflanagan8 |
#13 | interdiff_13-FAIL--13-NOT-FAIL.txt | 1.16 KB | danflanagan8 |
#13 | 3126746--13.patch | 5.74 KB | danflanagan8 |
#13 | 3126746--13--FAIL.patch | 4.58 KB | danflanagan8 |
Comments
Comment #2
geek-merlinPatch flying in that should fix it.
Comment #3
geek-merlinWhere this originated.
Comment #4
geek-merlinPatch fixes the bug for me.
Comment #5
tim.plunkettGood 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:
Missing space before {
{@inheritdoc}
Comment #6
Neslee Canil PintoMade requested changes from #5.
Comment #7
geek-merlinAll nits from #5 have been addressed, setting RTBC.
Comment #8
geek-merlinAdded followup.
Comment #9
tim.plunkettLowercase
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.
Comment #11
Deepak Goyal CreditAttribution: Deepak Goyal at Srijan | A Material+ Company for Drupal India Association commentedHi @tim.plunkett I have updated the patch please review.
Comment #12
tim.plunkettThanks for the capitalization fix. Still needs tests and a follow-up.
Comment #13
danflanagan8Here's a patch with a FAIL test followed by a patch that includes the code from #11 tested against the fail patch.
Comment #14
tim.plunkettTest looks great, just some nits from copy/paste:
Doesn't look like this is used
Don't need a variable for one usage.
From #5
Comment #15
danflanagan8Updated 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?
Comment #16
tim.plunkettI 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!
Comment #17
danflanagan8Thanks, @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.
Comment #19
larowlansaving issue credit
Comment #20
larowlanCommitted 6ffbfe2 and pushed to 9.1.x. Thanks!
Flagging for possible backport
Comment #21
larowlanComment #23
alexpott+1 for a backport.
Comment #25
larowlanBackported to 9.0.x