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

Command icon 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

eelkeblok created an issue. See original summary.

eelkeblok’s picture

Issue summary: View changes
eelkeblok’s picture

Title: Main forum overview form is an EntityForm but getEntity returns NULL » Main forum overview form is an EntityForm but does not implement interface properly

I 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.

eelkeblok’s picture

Status: Active » Needs review
eelkeblok’s picture

Issue summary: View changes
eelkeblok’s picture

Status: Needs review » Needs work

Ah, 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.

eelkeblok’s picture

Yes, 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.

larowlan’s picture

I'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

eelkeblok’s picture

OK, 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...

eelkeblok’s picture

It took a little bit of digging (git bisect is your friend), but this: https://www.drupal.org/node/3528300

eelkeblok’s picture

Status: Needs work » Needs review

I removed the redundant test and hopefully now made sure the code works on all supported core versions.

eelkeblok’s picture

Status: Needs review » Needs work

Not sure what is causing the last test failure.

eelkeblok’s picture

The 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".

eelkeblok’s picture

Status: Needs work » Needs review

I 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.

larowlan’s picture

Status: Needs review » Fixed

fixed thanks, I'll cut a release

Now that this issue is closed, review the contribution record.

As a contributor, attribute any organization that helped you, or if you volunteered your own time.

Maintainers, credit people who helped resolve this issue.

larowlan’s picture

  • larowlan committed ebf843cd on 1.x authored by eelkeblok
    fix: #3567960 Main forum overview form is an EntityForm but does not...

Status: Fixed » Closed (fixed)

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

eelkeblok’s picture

Sorry 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).

larowlan’s picture

Ah, so I did, cutting 1.0.6 now, thanks for the prod

larowlan’s picture