Problem/Motivation

Provide a replacement for form_load_include().

Proposed resolution

Remaining tasks

User interface changes

API changes

form_load_include() replaced with FormStateInterface::loadInclude()

CommentFileSizeAuthor
#16 2328785-16.patch626 bytesandypost
#12 form-2328785-11.patch11.11 KBtim.plunkett
#9 interdiff.txt837 bytestim.plunkett
#9 form-2328785-9.patch11.14 KBtim.plunkett
#7 form-2328785-7.patch11.15 KBtim.plunkett
#7 interdiff.txt5.76 KBtim.plunkett
#5 interdiff.txt3.19 KBtim.plunkett
#5 form-2328785-5.patch11.15 KBtim.plunkett
#2 interdiff.txt2.54 KBtim.plunkett
#2 form-2328785-2.patch11.13 KBtim.plunkett
#1 form-2328785-1-conversion.patch8.69 KBtim.plunkett
#1 form-2328785-1-conversion-and-removal.patch8.58 KBtim.plunkett
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
CreditAttribution: tim.plunkett commented
Status: Active » Needs review
FileSize
form-2328785-1-conversion-and-removal.patch8.58 KB
form-2328785-1-conversion.patch8.69 KB

Here are two patches. I'd prefer to remove the function now, but I'm not sure what the current preferred approach is.

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
CreditAttribution: tim.plunkett commented
FileSize
form-2328785-2.patch11.13 KB
interdiff.txt2.54 KB

Here it is with fully unit tests.

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented 
  1. +++ b/core/lib/Drupal/Core/Form/FormState.php
@@ -432,6 +432,29 @@ public function setFormState(array $form_state_additions) {
+      if ($result = $this->moduleLoadInclude($type, $module, $name)) {

@@ -824,4 +847,11 @@ protected function drupalSetMessage($message = NULL, $type = 'status', $repeat =
+  /**
+   * Wraps module_load_include().
+   */
+  protected function moduleLoadInclude($type, $module, $name = NULL) {
+    return module_load_include($type, $module, $name);
+  }

    public function loadInclude($module, $type, $name = NULL); just use the module handler 

  2. +++ b/core/tests/Drupal/Tests/Core/Form/FormStateTest.php
@@ -348,6 +348,79 @@ public function providerTestIsValueEmpty() {
+  /**
+   * @covers ::loadInclude
+   */
+  public function testLoadInclude() {
...
+  /**
+   * @covers ::loadInclude
+   */
+  public function testLoadIncludeNoName() {
...
+  /**
+   * @covers ::loadInclude
+   */
+  public function testLoadIncludeNotFound() {
...
+  /**
+   * @covers ::loadInclude
+   */
+  public function testLoadIncludeAlreadyLoaded() {

    <3 *I see what you did here!

tim.plunkett’s picture

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

Wow, I had no idea that module_load_include was replaced like that.
But this is FormState, we can't inject into it. I guess I'll just keep wrap the moduleHandler? :\

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
CreditAttribution: tim.plunkett commented
FileSize
form-2328785-5.patch11.15 KB
interdiff.txt3.19 KB

Meh.

Status: Needs review » Needs work

The last submitted patch, 5: form-2328785-5.patch, failed testing.

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
CreditAttribution: tim.plunkett commented
Status: Needs work » Needs review
FileSize
interdiff.txt5.76 KB
form-2328785-7.patch11.15 KB

Interdiff against #2.

jibran’s picture

jibran
he/him
Primary language Urdu
Location Sydney, Australia
CreditAttribution: jibran commented 
+++ b/core/tests/Drupal/Tests/Core/Form/FormStateTest.php
@@ -348,6 +348,79 @@ public function providerTestIsValueEmpty() {
+  public function testLoadInclude() {
...
+  public function testLoadIncludeNoName() {
...
+  public function testLoadIncludeNotFound() {

I'd suggest we should use dataProvider here for DRY code.

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
CreditAttribution: tim.plunkett commented
FileSize
form-2328785-9.patch11.14 KB
interdiff.txt837 bytes

I started with a data provider, but it resulted in very very complicated test method, more than 6 parameters, and only 4 cases.
So I decided that 4 test methods was much clearer to read.

I noticed a bad indent, just fixing that.

jibran’s picture

jibran
he/him
Primary language Urdu
Location Sydney, Australia
CreditAttribution: jibran commented
Status: Needs review » Reviewed & tested by the community

Ok then.

tim.plunkett’s picture

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

Rerolled after #2328777: Refactor FAPI getCache()/setCache() into a standalone class

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
CreditAttribution: tim.plunkett commented
FileSize
form-2328785-11.patch11.11 KB
webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
CreditAttribution: webchick commented
Status: Reviewed & tested by the community » Fixed

Committed and pushed to 8.x. Thanks!

  • webchick committed db30eaf on 8.0.x 
    Issue #2328785 by tim.plunkett: Convert form_load_include() to FormState...
andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost commented
Status: Fixed » Needs work
Issue tags: +Needs change record

This needs more work, probably in follow-up,
There's no change record

+++ b/core/lib/Drupal/Core/Form/FormState.php
@@ -432,6 +432,29 @@ public function setFormState(array $form_state_additions) {
+  public function loadInclude($module, $type, $name = NULL) {

+++ b/core/lib/Drupal/Core/Form/FormStateInterface.php
@@ -47,6 +47,37 @@ public function &getCompleteForm();
+   *   // Load node.admin.inc from Node module.
+   *   $form_state->loadInclude('inc', 'node', 'node.admin');
...
+  public function loadInclude($module, $type, $name = NULL);

+++ b/core/modules/ckeditor/src/Plugin/CKEditorPlugin/DrupalImage.php
@@ -68,7 +68,7 @@ public function getButtons() {
-    form_load_include($form_state, 'inc', 'editor', 'editor.admin');
+    $form_state->loadInclude('editor', 'admin.inc');

+++ b/core/modules/views_ui/src/ViewFormBase.php
@@ -35,7 +35,7 @@ public function init(FormStateInterface $form_state) {
-    form_load_include($form_state, 'inc', 'views_ui', 'admin');
+    $form_state->loadInclude('views_ui', 'inc', 'admin');

The example code is wrong!
The usage is confusing too...

andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost commented
Issue summary: View changes
Status: Needs work » Needs review
FileSize
2328785-16.patch626 bytes

Fix docs

andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost commented
Issue tags: -Needs change record

Updated https://www.drupal.org/node/2121003

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
CreditAttribution: tim.plunkett commented
Status: Needs review » Reviewed & tested by the community

Thanks @andypost!

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
CreditAttribution: webchick commented
Status: Reviewed & tested by the community » Fixed

Oops. This fell off my radar.

Committed and pushed to 8.x. Thanks!

  • webchick committed 9cd14f7 on 8.0.x 
    Issue #2328785 follow-up by andypost: Fix docs.

Status: Fixed » Closed (fixed)

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