Remove uses of form_execute_handlers(), which is deprecated. This will also make sure that we can more easily unit test entity forms as we can simple set up a custom container instead of having to declare the requires functions during the test. While this isn't the best end result, it is a major improvement to testability.

CommentFileSizeAuthor
#22 drupal_2278403_21.patch1.97 KBxano
#19 remove_uses_of-2278403-19.patch1.85 KBjeroent
#1 drupal_2278403_1.patch1.95 KBxano

Comments

xano’s picture

xano
he/they
Primary language English
Location Southampton
commented
Status: Active » Needs review
StatusFileSize
new drupal_2278403_1.patch1.95 KB
xano’s picture

xano
he/they
Primary language English
Location Southampton
commented
Parent issue: » #2205673: [META] Remove all @deprecated functions marked "remove before 8.0"
xano’s picture

xano
he/they
Primary language English
Location Southampton
commented
Issue summary: View changes
kim.pepper’s picture

kim.pepper
Primary language English
Location 🏄‍♂️🇦🇺Sydney, Australia
commented

This looks good.

Discussed with Xano in Austin sprint. The ideal solution would be to using constructor injection but that would blow out the scope of this issue. Suggest we create a follow up for that.

kim.pepper’s picture

kim.pepper
Primary language English
Location 🏄‍♂️🇦🇺Sydney, Australia
commented
Status: Needs review » Reviewed & tested by the community
alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Reviewed & tested by the community » Postponed

Let's postpone this one on #2278403: Remove uses of form_execute_handlers() - since that is suggesting we remove the service altogether.

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
commented

You linked to this issue. Which did you have in mind?

xano’s picture

xano
he/they
Primary language English
Location Southampton
commented
Status: Postponed » Needs review

By what I heard from @chx I *think* @alexpott meant #2282089: [security weakness] Hardwire FormValidator.

Moving back to NR, since the patch does not only change the invocation of validation handlers, but also those of submission handlers.

xano’s picture

xano
he/they
Primary language English
Location Southampton
commented

1: drupal_2278403_1.patch queued for re-testing.

Xano queued 1: drupal_2278403_1.patch for re-testing.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented

Yes I did mean #2282089: [security weakness] Hardwire FormValidator

xano’s picture

xano
he/they
Primary language English
Location Southampton
commented

Considering the lack of support and activity in that other issue, would you be okay with fixing this issue until #2282089: [security weakness] Hardwire FormValidator can possibly come up with a permanent solution?

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Needs review » Needs work

Well can we then wrap this on FormBase like we do in FormBase::errorHandler()

xano’s picture

xano
he/they
Primary language English
Location Southampton
commented

Does anyone know why #2004244-40: Move entity revision, content translation, validation and field methods to ContentEntityInterface added these lines and the code comments that precede them?

Status: Needs work » Needs review

Xano queued 1: drupal_2278403_1.patch for re-testing.

xano’s picture

xano
he/they
Primary language English
Location Southampton
commented

Well can we then wrap this on FormBase like we do in FormBase::errorHandler()

Do we really want to introduce new methods that are temporary anyway? We'd then have to mock the SUT, which works, but isn't the best solution. Setting the global container isn't great either, but at least UnitTestCase cleans that up nicely.

Xano queued 1: drupal_2278403_1.patch for re-testing.

Status: Needs review » Needs work

The last submitted patch, 1: drupal_2278403_1.patch, failed testing.

jeroent’s picture

jeroent
Primary language Dutch
Location 🇧🇪
commented
Status: Needs work » Needs review
StatusFileSize
new remove_uses_of-2278403-19.patch1.85 KB

Patch no longer applied. Created reroll.

Status: Needs review » Needs work

The last submitted patch, 19: remove_uses_of-2278403-19.patch, failed testing.

xano’s picture

xano
he/they
Primary language English
Location Southampton
commented
Status: Needs work » Needs review

Re-roll from #1.

Patch no longer applied. Created reroll.

Thanks! Apparently you did not create a proper re-roll. https://www.drupal.org/patch/reroll explains two different methods of doing this. It may be helpful to you.

xano’s picture

xano
he/they
Primary language English
Location Southampton
commented
StatusFileSize
new drupal_2278403_21.patch1.97 KB
rpayanm’s picture

rpayanm
he/him
Primary language Spanish
commented
Status: Needs review » Reviewed & tested by the community

Removed all occurrences. Only function exists.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Reviewed & tested by the community » Needs work

Can the change record that announces the replacement of form_execute_handlers be linked to this issue for traceability - once this is done it can be set back to rtbc.

jeroent’s picture

jeroent
Primary language Dutch
Location 🇧🇪
commented
Status: Needs work » Reviewed & tested by the community

Linked this issue to the following change record: drupal_*_form() and form_*() methods are replaced by a form builder service and methods on $form_state

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Reviewed & tested by the community » Fixed

This issue is a prioritized change as per #2350615: [policy, no patch] What changes can be accepted during the Drupal 8 beta phase? and it's benefits outweigh any disruption. Committed 29b678c and pushed to 8.0.x. Thanks!

  • alexpott committed 29b678c on 8.0.x 
    Issue #2278403 by Xano, JeroenT: Remove uses of form_execute_handlers().

Status: Fixed » Closed (fixed)

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