Problem/Motivation

A form used for the _entity_form route property must inherit from EntityForm, not FormBase.

This should be documented.

Steps to reproduce

Try it.
You get:

Error: Call to undefined method MyEntityForm::setModuleHandler() in Drupal\Core\Entity\EntityTypeManager->getFormObject() (line 230 of core/lib/Drupal/Core/Entity/EntityTypeManager.php).

Drupal\Core\Entity\EntityTypeManager->getFormObject('group', 'member-bulk-edit') (Line: 82)
Drupal\Core\Entity\HtmlEntityFormController->getFormObject(Object, 'group.member-bulk-edit.default') (Line: 76)
Drupal\Core\Controller\FormController->getContentResult(Object, Object)

Proposed resolution

Throw exception when form handler doesn't implement EntityFormInterface

Remaining tasks

Review

User interface changes

None

API changes

None

Data model changes

None

Release notes snippet

None

CommentFileSizeAuthor
#25 3205577-nr-bot_yuch2urw.txt91 bytesneeds-review-queue-bot

Issue fork drupal-3205577

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:

About issue forks

Comments

joachim created an issue. See original summary.

Version: 9.2.x-dev » 9.3.x-dev

Drupal 9.2.0-alpha1 will be released the week of May 3, 2021, which means new developments and disruptive changes should now be targeted for the 9.3.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.3.x-dev » 9.4.x-dev

Drupal 9.3.0-rc1 was released on November 26, 2021, which means new developments and disruptive changes should now be targeted for the 9.4.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.4.x-dev » 9.5.x-dev

Drupal 9.4.0-alpha1 was released on May 6, 2022, which means new developments and disruptive changes should now be targeted for the 9.5.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.5.x-dev » 10.1.x-dev

Drupal 9.5.0-beta2 and Drupal 10.0.0-beta2 were released on September 29, 2022, which means new developments and disruptive changes should now be targeted for the 10.1.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 10.1.x-dev » 11.x-dev

Drupal core is moving towards using a “main” branch. As an interim step, a new 11.x branch has been opened, as Drupal.org infrastructure cannot currently fully support a branch named main. New developments and disruptive changes should now be targeted for the 11.x branch, which currently accepts only minor-version allowed changes. For more information, see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

joachim’s picture

joachim commented
Issue tags: +Novice

Aditi Saraf made their first commit to this issue’s fork.

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented

Should we add a runtime assertion so anyone who makes this mistake gets a better error message?

aditi saraf’s picture

aditi saraf commented
Assigned: Unassigned » aditi saraf

i will work on this ..

aditi saraf’s picture

aditi saraf commented
Assigned: aditi saraf » Unassigned

@Joachim i am not able to reproduce this issue .

joachim’s picture

joachim commented

I can still see the call to setModuleHandler in getFormObject(), which AFAICT isn't in FormBase.

xdequinze’s picture

xdequinze
Location Brussels
commented

@joachim, I am trying to understand the issue. Would you like to provide more details about it ? What are you trying to achieve ?

joachim’s picture

joachim commented

If you define a route that uses an entity form, you use '_entity_form' in the route definition, and set a form class name as a value.

If your form class inherits the standard form class base, it crashes.

The docs for _entity_form in entity.api.php should say that you need to use a specific base class.

anjali rathod’s picture

anjali rathod
Primary language English
Location India
commented
Issue tags: +Singapore2024

acbramley made their first commit to this issue’s fork.

acbramley opened merge request !12306

acbramley’s picture

acbramley commented
Issue summary: View changes
Status: Active » Needs review
Issue tags: -Novice

Encountered this in #3461700: Allow vocabulary overview form class to be overridden and I agree we should improve this by throwing an error. I don't think this is novice based on the test coverage required.

acbramley’s picture

acbramley commented
Related issues: +#3461700: Allow vocabulary overview form class to be overridden
joachim’s picture

joachim commented
Status: Needs review » Needs work

The check in core/lib/Drupal/Core/Entity/EntityTypeManager.php is not what I was expecting, and it's a good idea because it'll give a better error for people who use the wrong sort of form class as an entity handler.

But it won't catch the case where you use _entity_form directly in a route definition.

acbramley’s picture

acbramley commented
Status: Needs work » Needs review

It does catch it, try adding this (mimics VocabularyRouteProvider::getOverviewPageRoute)

entity.vocab.overview:
  path: '/admin/structure/taxonomy/manage/{taxonomy_vocabulary}/overview2'
  defaults:
    _entity_form: 'taxonomy_vocabulary.overview'
    _title: 'Test'
  requirements:
    _entity_access: 'taxonomy_vocabulary.access taxonomy overview'
  options:
    _admin_route: TRUE
  parameters:
    taxonomy_vocabulary:
      with_config_overrides: TRUE

EntityRouteEnhancer::enhanceEntityForm sets the controller to controller.entity_form:getContentResult based on the _entity_form default. That calls HtmlEntityFormController::getFormObject which calls EntityTypeManager::getFormObject

The _entity_form route default is tied directly to the form handler class.

joachim’s picture

joachim commented

Ah, I hadn't spotted that!

The exception message will be wrong in that scenario though:

> 'The "%s" form handler of the "%s" entity type specifies a class "%s" that does not extend "%s".'

because it's not been caused by a form handler.

acbramley’s picture

acbramley commented

because it's not been caused by a form handler.

The form handler is the second part of the _entity_form string, e.g the example above maps to the overview form handler of the Vocabulary entity type.
I also wondered if we should just through the error when the entity type is parsed, but technically at the moment you can have form handlers that aren't tied to routes and therefore don't need to implement the interface.
In Vocabulary's case it had the form handler but the route for it isn't using the _entity_form method and afaik form handlers aren't used in many other ways?
We also might need to throw a deprecation first before a full exception so sites don't blow up?

joachim’s picture

joachim commented

> I also wondered if we should just through the error when the entity type is parsed, but technically at the moment you can have form handlers that aren't tied to routes and therefore don't need to implement the interface.

In that scenario, is there something like DefaultHtmlRouteProvider setting up the route?

Or are there entity types that specify a form handler for a form that's only shown embedded in something else?

needs-review-queue-bot’s picture

needs-review-queue-bot commented
Status: Needs review » Needs work
StatusFileSize
new 3205577-nr-bot_yuch2urw.txt91 bytes

The Needs Review Queue Bot tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

acbramley’s picture

acbramley commented
Status: Needs work » Needs review

smustgrave made their first commit to this issue’s fork.

smustgrave’s picture

smustgrave commented
Title: EntityRouteEnhancer and HtmlEntityFormController should document requirements for the _entity_form property » Form handlers should throw exception when not implementing EntityFormInterface
Component: documentation » entity system
Status: Needs review » Reviewed & tested by the community
Issue tags: +Needs Review Queue Initiative

Reviewed the CR and pretty straight forward and warns it must use EntityFormInterface

Test coverage is here https://git.drupalcode.org/issue/drupal-3205577/-/jobs/7267720

Fixed the title and component best I could

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented
Version: 11.x-dev » 11.3.x-dev
Status: Reviewed & tested by the community » Fixed

As it has a CR and there is an outside chance someone was doing something weird before, let's get this into 11.x/11.3.x only.

Committed and pushed 9976cfec451 to 11.x and 6e88a2d4084 to 11.3.x. Thanks!

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.

  • longwave committed 6e88a2d4 on 11.3.x 
    fix: #3205577 Form handlers should throw exception when not implementing...

  • longwave committed 9976cfec on 11.x 
    fix: #3205577 Form handlers should throw exception when not implementing...

longwave closed merge request !12306

joachim’s picture

joachim commented

I'm not sure we needed a CR here!

Previously, using the wrong form caused a PHP error -- ie a crash. Now it causes an exception with a clearer message -- still a crash (but a better one).

> Previously, if a form handler was used in an _entity_form route definition and didn't extend EntityForm, an obscure error was reported

'reported' implies it was logged, rather than what it was -- a crash.

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented

It's probably unlikely but if we didn't check the interface then it's still possible someone has implemented the necessary methods without it? Still, it's largely a DX improvement and 11.3 is out next month so I think it makes little difference.

Status: Fixed » Closed (fixed)

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