Problem/Motivation
The main forum overview form (Drupal\forum\Form\Overview) extends the OverviewTerms form from the Taxonomy module, which in turn is an EntityForm. The EntityFormInterface has a getEntity method, which, according to the interface defininition, should return a Drupal\Core\Entity\EntityInterface (and not NULL). For the forums overview form, that entity is never set, so the method does return NULL.
Normally, I would not be too worried about this, but the site I'm working on has Rabbit Hole installed. This contains a form alter, which checks if the form is an EntityFormInterface and then, you guessed it, calls getEntity(). One might argue that Rabbit Hole should then check if it got an entity before proceeding. One might also argue that Drupal\forum\Form\Overview should stick to its contract and return an entity from its getEntity() method.
Similarly, Simple XML Sitemap will expect an operation to be set for the form, which also is not set in this case.
Steps to reproduce
The easiest is to install Rabbit Hole and Simple XML Sitemap (best enable them separately to see the individual issues) and visit /admin/structure/forum. Maybe there are other situations where this is a problem, though.
Proposed resolution
Return the forum vocabulary from the getEntity() method for the Overview form. I doubt this would ever have any ill effects, and means the Interface is more accurately implemented.
Remaining tasks
- Create MR
- Review
- Merge
User interface changes
None. Well, the UI will actually work when Rabbit Hole is installed.
API changes
The getEntity() method for the Overview form will now return the forums vocabulary.
Data model changes
None.
Issue fork forum-3567960
Show commands
Start within a Git clone of the project using the version control instructions.
Or, if you do not have SSH keys set up on git.drupalcode.org:
Comments
Comment #2
eelkeblokComment #3
eelkeblokI renamed the issue because after resolving the issue with Rabbit Hole, I ran into the next issue, with Simple Sitemap, because the operation is not set.
Comment #5
eelkeblokComment #6
eelkeblokComment #7
eelkeblokAh, I forgot to commit the dev dependencies. But having given it a bit more thought, it might be better to just test the return values of the methods and not involve some fairly arbitrary third-party modules.
Comment #8
eelkeblokYes, I have considered the same thing. The reasoning behind doing it the way I did was not extremely thoroughly considered, but was something along the lines of, this form is only ever meant to edit the forums vocabulary (as configured in the configuration), so it makes a certain amount of sense that is what it leans into. But I'm happy to refactor to a separate controller too.
Comment #9
larowlanI'm pragmatic about it, your approach is doing the same thing but in a just in time manner.
I think we need to add properties for entity and operation (at least according to phpstan) and duplicates some existing tests, but other than that I'm happy with your approach
Comment #10
eelkeblokOK, this took far too long to land in my brain, but the OverviewTerms form from taxonomy was not extending EntityForm in D10 [or D11.1 and .2 for that matter]. That explains why I ran into this after updating to D11...
Comment #11
eelkeblokIt took a little bit of digging (git bisect is your friend), but this: https://www.drupal.org/node/3528300
Comment #12
eelkeblokI removed the redundant test and hopefully now made sure the code works on all supported core versions.
Comment #13
eelkeblokNot sure what is causing the last test failure.
Comment #14
eelkeblokThe error in the test is
Error: Undefined constant "Drupal\node\Entity\DRUPAL_OPTIONAL"That does crop up here and there, like #3396324: Undefined constant "Drupal\node\Entity\DRUPAL_OPTIONAL" and #3304657: Undefined constant "Drupal\node\Entity\DRUPAL_OPTIONAL".
Comment #15
eelkeblokI think this is now good to go. The test failures in the next minor seem unrelated. Looks like next minor was bumped to 11.4 since the last weekly build of the 1.x branch.
Comment #16
larowlanfixed thanks, I'll cut a release
Comment #18
larowlanhttps://www.drupal.org/project/forum/releases/1.0.5
Comment #21
eelkeblokSorry for commenting in this closed issue, but I only just now got to test the release. Unfortunately, the release tag is one commit short of including this issue in the 1.0.5 release (contrary to what the release notes say).
Comment #22
larowlanAh, so I did, cutting 1.0.6 now, thanks for the prod
Comment #23
larowlanhttps://www.drupal.org/project/forum/releases/1.0.6