Problem/Motivation

When a theme has both a Managed File field and a Submit callback like this:

/**
 * Implements hook_form_FORM_ID_alter().
 */
function THEME_form_system_theme_settings_alter(&$form, &$form_state) {
   
  // Adding field with Managed File.
  $form['custom_logo'] = [
    '#type' => 'managed_file',
    '#title' => t('Upload image for custom logo'),
    '#upload_location' => 'public://images/',
    '#default_value' => theme_get_setting('custom_logo'),
  ];

  // Add your submission handler to the form.
  $form['#submit'][] = 'THEME_form_system_theme_settings_submit';

}

/**
 * Theme Settings Submit Callback.
 */
function THEME_form_system_theme_settings_submit($form, &$form_state) {
  drupal_set_message('hello!');
}

results in this error:

Warning: call_user_func_array() expects parameter 1 to be a valid callback, function 'theme_form_system_theme_settings_submit' not found or invalid function name in Drupal\Core\Form\FormSubmitter->executeSubmitHandlers() (line 111 of core\lib\Drupal\Core\Form\FormSubmitter.php).

Proposed resolution

The reason for this is that the addition of files with processing functions is done using require_once:

# See Drupal\system\Form\ThemeSettingsForm::buildForm()

// Include the theme-settings.php file.
        $filename = DRUPAL_ROOT . '/' . $themes[$theme]->getPath() . '/theme-settings.php';
        if (file_exists($filename)) {
          require_once $filename;
        }

And only for uncached forms.

So you need to add files to buildInfo to ensure that they are available. In addition to the theme-settings.php, it will be useful to add a theme-name.theme that is also widely used by developers.

Remaining tasks

Workaround

We can add the necessary files to the buildInfo directly in the custom theme, like:

function THEME_form_system_theme_settings_alter(&$form, &$form_state) {
  # form settings here
  # ...

  $form['#submit'][] = 'THEME_form_system_theme_settings_submit';

  $theme = \Drupal::theme()->getActiveTheme()->getName();
  // Example for theme-settings.php file.
  $theme_file = drupal_get_path('theme', $theme) . '/theme-settings.php';
  
  $build_info = $form_state->getBuildInfo();
  if (!in_array($theme_file, $build_info['files'])) {
    $build_info['files'][] = $theme_file;
  }
  $form_state->setBuildInfo($build_info);
}

User interface changes

API changes

Data model changes

Original post

When a theme has both a Managed File field and a Submit callback like this:

/**
 * Implements hook_form_FORM_ID_alter().
 */
function THEME_form_system_theme_settings_alter(&$form, &$form_state) {
  
  $settings = theme_get_setting('theme');
      
  $form['theme'] = array(
    '#type' => 'container',
    '#tree' => TRUE,
  );
 
  $form['theme']['secondary_menu'] = array(
    '#type' => 'fieldset',
    '#title' => t('Secondary Menu'),
  );
  
  // Adding a Managed File makes the submit handler Fail
  $form['theme']['secondary_menu']['background_image'] = array(
    '#type' => 'managed_file',
    '#title' => t('Background Image'),
    '#default_value' => !empty($settings['secondary_menu']['background_image']) ? $settings['secondary_menu']['background_image'] : NULL,
    '#upload_location' => 'rcf://theme/',
  );
  
  // Add your submission handler to the form.
  $form['#submit'][] = 'THEME_form_system_theme_settings_submit';

}

/**
 * Theme Settings Submit Callback.
 */
function THEME_form_system_theme_settings_submit($form, &$form_state) {
  drupal_set_message('hello!');
}

results in this error:

 Fatal error: Call to undefined function THEME_form_system_theme_settings_submit() in includes/form.inc on line 1464

Note: Replace "THEME" and 'theme' with your theme name to duplicate the error.

thanks!
david


Remaining Tasks:

  1. Needs tests Done!
  2. Commit patch #1862892-148: When a theme has a managed_file field and a submit callback, call to undefined function error is thrown.
CommentFileSizeAuthor
#162 1862892-162.patch9.15 KBcilefen
#162 1862892-interdiff-158.txt1.85 KBcilefen
#158 interdiff-158-fix-only.txt1.36 KBAnonymous (not verified)
#158 1862892-158.patch9.17 KBAnonymous (not verified)
#158 1862892-158-test-only.patch7.81 KBAnonymous (not verified)
#6 theme-settings-in-build-info-1862892-6.patch902 bytespeterpoe
#7 theme-settings-in-build-info-1862892-7.patch942 bytespeterpoe
#18 theme-settings-in-build-info-1862892-18-D7.patch667 bytesG.I.Joe
#18 interdiff-1862892-7-18.txt1.17 KBG.I.Joe
#20 theme-settings-in-build-info-1862892-19-D7.patch573 bytesG.I.Joe
#20 interdiff-1862892-7-19.txt1002 bytesG.I.Joe
#21 theme-settings-in-build-info-1862892-21-D7.patch573 bytesG.I.Joe
#21 interdiff-1862892-7-21.txt1002 bytesG.I.Joe
#26 theme-settings-in-build-info-1862892-26-D7.patch788 bytesG.I.Joe
#26 1862892-21-26.txt638 bytesG.I.Joe
#32 theme-settings-in-build-info-1862892-32-D8.patch871 bytesleopathu
#39 custom-theme-settings-1862892-39-onlytests.patch5.01 KBoriol_e9g
#42 custom-theme-settings-1862892-42-onlytests.patch6.05 KBoriol_e9g
#44 custom-theme-settings-1862892-44-onlytests.patch6.05 KBoriol_e9g
#46 custom-theme-settings-1862892-46-onlytests.patch6.39 KBoriol_e9g
#50 custom-theme-settings-1862892-50.patch7.29 KBoriol_e9g
#55 settings-theme-managed-file-tests-1862892.patch10.95 KBoriol_e9g
#59 theme-settings-in-build-info-1862892-56-D8.patch1.14 KBleopathu
#62 theme-settings-in-build-info-1862892-62-D8.patch1.14 KBleopathu
#81 d8-theme_settings_in_build_info_patch62fix_with_32fortests-1862892-81.patch12.89 KBjoseph.olstad
#87 d8-theme_settings_in_build_info_patch62fix_with_32fortests_IMPROVED-1862892-87.patch12.5 KBjoseph.olstad
#87 interdiff_81_to_87.txt3.85 KBjoseph.olstad
#90 interdiff_81_to_90.txt3.84 KBjoseph.olstad
#90 d8-theme_settings_in_build_info_patch62fix_with_32fortests_IMPROVED-1862892-90.patch12.48 KBjoseph.olstad
#95 d8-theme_settings_in_build_info_patch62fix_with_32fortests_IMPROVED-1862892-95.patch12.5 KBjoseph.olstad
#95 interdiff_81_to_95.txt3.85 KBjoseph.olstad
#101 1862892-100.patch12.34 KBoriol_e9g
#101 interdiff.patch5.14 KBoriol_e9g
#104 1862892.patch12.38 KBoriol_e9g
#116 1862892-107.patch12.17 KBoriol_e9g
#116 interdiff-107-95.patch12.12 KBoriol_e9g
#121 interdiff_107_to_121.txt2.58 KBjoseph.olstad
#121 1862892-121.patch12.28 KBjoseph.olstad
#122 interdiff_121_to_122.txt953 bytesjoseph.olstad
#122 1862892-122.patch12.16 KBjoseph.olstad
#124 tests_only_failure_means_success121-1862892-124.patch11.01 KBjoseph.olstad
#132 interdiff_122_132.txt536 bytesjoseph.olstad
#132 tests_only_1862892-132.patch11.05 KBjoseph.olstad
#132 1862892-132.patch12.2 KBjoseph.olstad
#135 wtf.patch538 bytesoriol_e9g
#144 1862892-144-test-only.patch7.65 KBAnonymous (not verified)
#145 1862892-145.patch9.01 KBAnonymous (not verified)
#145 interdiff-144-145.txt1.36 KBAnonymous (not verified)
#145 interdiff-132-145.txt19.56 KBAnonymous (not verified)
#148 1862892-148.patch9.17 KBAnonymous (not verified)
#148 interdiff-145-148.txt1.03 KBAnonymous (not verified)
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

apaderno’s picture

Priority: Major » Normal
alisamar’s picture

Version: 7.x-dev » 7.19

validation functions also seems to be undefined.

An AJAX HTTP request terminated abnormally
Fatal error:  Call to undefined function _custom_validate() in /src/includes/form.inc on line 1410
apaderno’s picture

Version: 7.19 » 7.x-dev
pschuelke’s picture

I've been getting this same error. The code above is what I have. I've also read and tried the suggestions from here to no avail.

peterpoe’s picture

I have this long (but not too ugly) workaround. Just place the following in a custom module:


/**
 * Implements hook_form_FORM_ID_alter() for system_theme_settings().
 *
 * Workaround for bug https://drupal.org/node/1862892.
 */
function MYMODULE_form_system_theme_settings_alter(&$form, &$form_state) {
  if ($form['var']['#value'] != 'theme_settings') {
    $form['#validate_store'] = $form['#validate'];
    $form['#validate'] = array('MYMODULE_system_theme_settings_validate');
    $form['#submit_store'] = $form['#submit'];
    $form['#submit'] = array('MYMODULE_system_theme_settings_submit');
  }
}

/**
 * Additional validation handler for the theme settings form.
 */
function MYMODULE_system_theme_settings_validate(&$form, &$form_state) {
  MYMODULE_system_theme_settings_execute_handlers($form, $form_state, 'validate');
}

/**
 * Additional submission handler for the theme settings form.
 */
function MYMODULE_system_theme_settings_submit(&$form, &$form_state) {
  MYMODULE_system_theme_settings_execute_handlers($form, $form_state, 'submit');
}

/**
 * Executes validate and submit handlers for the theme settings form ensuring
 * that theme specific settings files are included first.
 *
 * @param $op
 *   The handler to invoke: "validate" or "submit".
 */
function MYMODULE_system_theme_settings_execute_handlers(&$form, &$form_state, $op) {
  $key = preg_replace(array('/^theme_/', '/_settings$/'), '', $form['var']['#value']);
  $themes = list_themes();

  if (isset($themes[$key]->base_themes)) {
    $theme_keys = array_keys($themes[$key]->base_themes);
    $theme_keys[] = $key;
  }
  else {
    $theme_keys = array($key);
  }

  $default_theme = !empty($GLOBALS['theme_key']) ? $GLOBALS['theme_key'] : NULL;
  $GLOBALS['theme_key'] = $key;

  foreach ($theme_keys as $theme) {
    $filename = DRUPAL_ROOT . '/' . str_replace("/$theme.info", '', $themes[$theme]->filename) . '/theme-settings.php';
    if (file_exists($filename)) {
      require_once($filename);
    }
  }

  // Execute the handler.
  foreach ($form['#' . $op . '_store'] as $function) {
    $function($form, $form_state);
  }

  if (isset($default_theme)) {
    $GLOBALS['theme_key'] = $default_theme;
  }
  else {
    unset($GLOBALS['theme_key']);
  }
}

/**
 * Implements hook_module_implements_alter().
 */
function MYMODULE_module_implements_alter(&$implementations, $hook) {
  // Ensure that MYMODULE_form_system_theme_settings_alter is
  // executed last. 
  if ($hook == 'form_alter') {
    $group = $implementations['MYMODULE'];
    unset($implementations['MYMODULE']);
    $implementations['MYMODULE'] = $group;
  }
}

peterpoe’s picture

Status: Active » Needs review
FileSize
902 bytes

The problem is that system_theme_settings() includes the theme-settings.php files with a simple require_once, so when the built form is reused on subsequent request the files are not reincluded.
There is a nice solution though: $form_state['build_info']['files'] is used by Form API internally to store files required by the form. We can just add the theme-settings.php files to the array.

If you need a workaround (please ignore #5, this is much better):

/**
 * Implements hook_form_FORM_ID_alter() for system_theme_settings().
 *
 * Workaround for bug https://drupal.org/node/1862892.
 */
function MYMODULE_form_system_theme_settings_alter($form, &$form_state) {
  if ($form['var']['#value'] != 'theme_settings') {
    $key = preg_replace(array('/^theme_/', '/_settings$/'), '', $form['var']['#value']);
    $themes = list_themes();

    if (isset($themes[$key]->base_themes)) {
      $theme_keys = array_keys($themes[$key]->base_themes);
      $theme_keys[] = $key;
    }
    else {
      $theme_keys = array($key);
    }

    foreach ($theme_keys as $theme) {
      $theme_path = drupal_get_path('theme', $theme);
      if (file_exists($theme_path . '/theme-settings.php')) {
        $form_state['build_info']['files'][] = $theme_path . '/theme-settings.php';
      }
    }
  }
}
peterpoe’s picture

Updated patch with correct path to file(s).

joaogarin’s picture

Hello,

Is there a way to fix this without a custom module? A solution inside the theme itself? I am building a theme and I can not "force" the user to install any specific module so I would have to address this issue in the theme itself.

Thank you,
best regards,
Joao Garin

pdcarto’s picture

This seems to be working for me inside my theme's MYTHEME_form_system_theme_settings_alter function in my theme-settings.php:

  // Work-around for this bug: https://drupal.org/node/1862892
  $theme_settings_path = drupal_get_path('theme', 'MYTHEME') . '/theme-settings.php';
  if (file_exists($theme_settings_path) && !in_array($theme__settings_path, $form_state['build_info']['files'])) {
    $form_state['build_info']['files'][] = $theme_settings_path;
  }
aaronschachter’s picture

We got #9 to work but there is a misspelling, it should read as:

if (file_exists($theme_settings_path) && !in_array($theme_settings_path, $form_state['build_info']['files'])) {

Make sure that the two parameters for the function are (&$form, &$form_state) -- this was not working at first because we were passing the parameters (&$form, $form_state)

Talkless’s picture

Thanks for walkaround.

I wonder, how is it with Drupal 8? Maybe there will be simpler way to upload permanent file in theme settings?

robynlgreen’s picture

Just a head up for anyone using Omega, the fix in #9 works, but you get an error on save/submit:

"Fatal error: Call to undefined function omega_extension_development_settings_form_submit() in /xxx/drupal/includes/form.inc on line 1465"

There's an issue queue on this and patches for a fix here: https://www.drupal.org/node/2186031

damonbla’s picture

Good to find this thread, because it means the hours I've been banging my head against my screen weren't because I was insane, it's because there's a bug!

The problem now is I've tried #9 and it does not work for me. I'm on an Omega subtheme too, and I'm not receiving the error mentioned in #12 either. The whole thing just stops in the middle of the custom submit function. It does go farther than it did before with #9 because now my custom validation function runs without trouble, but the submit still doesn't quite get there.

Anyone else worked on this or figured out a good final solution?

basant’s picture

I am using 7.38-dev version, and the above code works perfectly fine on it. I am using theme "seven". On saving the form it uploads the file successfully with message "!hello". I have used upload location as "public://theme/" instead of rcf.

nabajit’s picture

Added #9 and #10 in my theme settings which worked like a charm for me.

Thanks all.

deggertsen’s picture

The patch in #7 fixed the issue for me. Obviously not wanting to patch core though if possible. Unfortunately none of the other solutions worked for me.

leopathu’s picture

Same thing happening in Drupal 8 too,

Warning: call_user_func_array() expects parameter 1 to be a valid callback, function 'integrity_settings_form_submit' not found or invalid function name in Drupal\Core\Form\FormSubmitter->executeSubmitHandlers() (line 116 of /home/leopathu/Public/drupal-8.0.0/core/lib/Drupal/Core/Form/FormSubmitter.php).

Any solutions ?

G.I.Joe’s picture

Added a new patch with simplified code.
This patch and interdiff is useless. Was created with a color format.

Status: Needs review » Needs work

The last submitted patch, 18: theme-settings-in-build-info-1862892-18-D7.patch, failed testing.

G.I.Joe’s picture

Added a new patch with simplified code.
Wrong comment_number.

G.I.Joe’s picture

Added a new patch with simplified code.

G.I.Joe’s picture

Status: Needs work » Needs review
CProfessionals’s picture

I had a similar problem with Corporate3X Theme. I applied #9. Just to be a little clearer after pasting the code:

  1. Fix the "typo" and remove second underscore: $theme__settings_path ----> $theme_settings_path
  2. Change 'MYTHEME' to the name of your theme. In my case it was 'corporatex3'

Hope this helps.
Thanks for figuring this out!

SchwebDesign’s picture

tested #21 but it gave error:

Fatal error:  require_once(): Failed opening required '/home/account/public_html//home/account/public_html/sites/all/themes/nrgtravel/theme-settings.php' (include_path='.:/usr/lib/php:/usr/local/lib/php') in /home/bealem/public_html/includes/form.inc on line 535

changing line 565
from

      $filename = DRUPAL_ROOT . '/' . str_replace("/$theme.info", '', $themes[$theme]->filename) . '/theme-settings.php';

to

      $filename = DRUPAL_ROOT . str_replace("/$theme.info", '', $themes[$theme]->filename) . '/theme-settings.php';

seemed to fix that error
and then this patch worked for me

danquah’s picture

Status: Needs review » Reviewed & tested by the community

While simpler, the patch in #21 does not work (at the very least not in all cases) as it uses the absolute path to theme-settings.php

Just tested #7 and it works as advertised. It also reads easier as it uses drupal_get_path instead of str_replace'ing the path to the themes info-file.

The latter approach might be slightly faster, but that can hardly be an issue in code like this.

G.I.Joe’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
788 bytes
638 bytes

good point. Created new patch. please, review.
ok never mind. :-)

Status: Needs review » Needs work

The last submitted patch, 26: theme-settings-in-build-info-1862892-26-D7.patch, failed testing.

G.I.Joe’s picture

Status: Needs work » Reviewed & tested by the community
Fabianx’s picture

Version: 7.x-dev » 8.1.x-dev
Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs tests

+1 to adding this to build_info, but:

a) This needs to be fixed in D8 first.
b) This needs tests as people have shown the error can be reproduced easily.

David_Rothstein’s picture

Issue tags: +Needs backport to D7
leopathu’s picture

If I write the theme settings alter in THEMENAME.theme file, It would also getting the same error, but the patch #28 includes only the theme-settings.php file . Is it enough in drupal 8 ?

leopathu’s picture

Status: Needs work » Needs review
FileSize
871 bytes

I hope, This patch would fix the issue in drupal 8, But Needs to be more discuss on comment #31

leopathu’s picture

Priority: Normal » Major

Version: 8.1.x-dev » 8.2.x-dev

Drupal 8.1.9 was released on September 7 and is the final bugfix release for the Drupal 8.1.x series. Drupal 8.1.x will not receive any further development aside from security fixes. Drupal 8.2.0-rc1 is now available and sites should prepare to upgrade to 8.2.0.

Bug reports should be targeted against the 8.2.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

joelpittet’s picture

+++ b/core/modules/system/src/Form/ThemeSettingsForm.php
@@ -315,9 +315,10 @@ public function buildForm(array $form, FormStateInterface $form_state, $theme =
-        $filename = DRUPAL_ROOT . '/' . $themes[$theme]->getPath() . '/theme-settings.php';
+        $filename = drupal_get_path('theme', $theme) . '/theme-settings.php';

Is this needed still? I expect getPath() works fine. The real fix is the other line.

Anybody interested in writing a test for this?

joelpittet’s picture

Status: Needs review » Postponed (maintainer needs more info)

for #35

Fabianx’s picture

Status: Postponed (maintainer needs more info) » Needs work

Definitely needs more work than information; without a test this is CNW as there are actionable items.

With a test that line can also be removed and seen if tests are still passing.

oriol_e9g’s picture

We have the same problem with a custom sub-template and I can confirm that the patch in #32 solves the problem.

For the proper fix the two changes are necesary.

In our case, this:
$filename = DRUPAL_ROOT . '/' . $themes[$theme]->getPath() . '/theme-settings.php';
Returns:
/var/local/html/drupal8/node1/sites/***/themes/bootstrap/theme-settings.php
And the file_exists($filename) fails.

With the change:
$filename = drupal_get_path('theme', $theme) . '/theme-settings.php';
Returns:
sites/***/themes/bootstrap/theme-settings.php
And the file_exists($filename) pass.

oriol_e9g’s picture

Status: Needs work » Needs review
FileSize
5.01 KB

I'm trying to add some tests, but they are not executing locally... I will appreciate some guidance :S

oriol_e9g’s picture

Assigned: Unassigned » oriol_e9g

Status: Needs review » Needs work

The last submitted patch, 39: custom-theme-settings-1862892-39-onlytests.patch, failed testing.

oriol_e9g’s picture

Status: Needs work » Needs review
FileSize
6.05 KB

Status: Needs review » Needs work

The last submitted patch, 42: custom-theme-settings-1862892-42-onlytests.patch, failed testing.

oriol_e9g’s picture

Maybe I have to use WebTestBase instead of BrowserTestBase?

joelpittet’s picture

WebTestBase or KernalTestBase should do the trick. BrowserTestBase is more for JS FWIU

oriol_e9g’s picture

Tests progress, still needs work.

The "managed_file" field it's not appearing in the settings form and I don't kwo why :S

Status: Needs review » Needs work

The last submitted patch, 46: custom-theme-settings-1862892-46-onlytests.patch, failed testing.

leopathu’s picture

What do you think about the #31 Comment ?

oriol_e9g’s picture

If both cases are supported we need to fix both cases... but first we have to work in tests.

If we can create a proper test using a dummy theme with theme-settings.php, then we can replicate the same test with the hooks in THEMENAME.theme

I don't kwow if the use of theme-settings.php is deprecated in favour of THEMENAME.theme

oriol_e9g’s picture

Initial tests.

I have discovered that define the config schema with the new fields is mandatory for passing the test: config/schema/THEMENAME.schema.yml

This is not described in documentation: https://www.drupal.org/docs/8/theming-drupal-8/creating-advanced-theme-s...

If we want this, we should update the documentation.

oriol_e9g’s picture

Assigned: oriol_e9g » Unassigned
Status: Needs work » Needs review

I can not work on it for a while, maybe someone else wants to complete the work in the meantime.

Status: Needs review » Needs work

The last submitted patch, 50: custom-theme-settings-1862892-50.patch, failed testing.

oriol_e9g’s picture

Title: When theme-settings.php has a managed_file field and a submit callback, " Call to undefined function" error is thrown. » When a theme has a managed_file field and a submit callback, call to undefined function error is thrown.

I have found a contrib theme afected with the same bug, they define the submit handler in THEMENAME.theme file https://www.drupal.org/node/2672680

oriol_e9g’s picture

The same bug it's also related here: https://www.drupal.org/node/2779947

oriol_e9g’s picture

Status: Needs work » Needs review
FileSize
10.95 KB

The complet test coverage should be something like this tests.

  1. I have created new dummy theme with a theme-settings.php file
  2. I have add some custom fields with a managed_file and a submit handler using hook_form_system_theme_settings_alter
  3. The first testing theme call the hooks in THEMENAME.theme
  4. The other theme call the hooks in theme-settings.php
  5. The tests try to add a new image file using the ajax upload button from managed_file
  6. Validates that the file is uploaded as a temporary file
  7. Then submits the form as a normal post
  8. Finally validates that the submit handler is fired and the attached file is changed to permanent

We still need to find a solution for the THEMENAME.theme case.

Status: Needs review » Needs work

The last submitted patch, 55: settings-theme-managed-file-tests-1862892.patch, failed testing.

oriol_e9g’s picture

oriol_e9g’s picture

leopathu’s picture

i have added the THEMENAME.theme file in this patch.
I am not sure this is the best way to add the THEMENAME.theme, needs proper review.

The last submitted patch, , failed testing.

Status: Needs review » Needs work

The last submitted patch, 59: theme-settings-in-build-info-1862892-56-D8.patch, failed testing.

leopathu’s picture

Status: Needs work » Needs review
FileSize
1.14 KB

i hope, this patch would pass the test.

oriol_e9g’s picture

Anybody knows why test always throw this error?

{"message":"A fatal error occurred: The specified #ajax callback is empty or not callable."}

leopathu’s picture

Status: Needs review » Needs work
leopathu’s picture

I think, @oriol_e9g This #63 is happening because of this (handling in this page) issue. but not sure.

xjm’s picture

Issue tags: +Triaged core major

@lauriii, @joelpittet, @Cottser, @alexpott, and I discussed this issue awhile back and agreed to handle it as a major bug. While the fatal error cannot be triggered through the core UI alone, it happens unexpectedly with very simple, apparently supported code, and is a nontrivial DX issue since the cause is so non-obvious.

Thanks for working on this!

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.6 was released on February 1, 2017 and is the final full bugfix release for the Drupal 8.2.x series. Drupal 8.2.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.3.0 on April 5, 2017. (Drupal 8.3.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.3.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

joseph.olstad’s picture

If you wonder why people are ditching Drupal for wordpress, it's because there's very few working themes available.
we need this patch asap, this is preventing hundreds of contrib themes from working correctly with newer versions of drupal.

All of the following contrib themes have been broken since very early releases of D7 somewhere a regression came in and the easiest way to fix is to apply the core patch in this issue

Please get this in D8 and D7 asap.

RTBC patch #7

related issue:
#2884164: theme no longer behaves since upgrading from media <=2.0-alpha3

Michael-IDA’s picture

Housekeeping. Adding all the linked/related issues for their visibility.

This bug has been an direct issue for my client for the last ~3 months, and was opened 5 years ago?

This isn’t showing up on Google searches, so adding this for search results.

Fatal error: Call to undefined function theme1033_form_system_theme_settings_reset() in includes/form.inc on line 1520

Fatal error: Call to undefined function theme1033_form_system_theme_settings_submit() in includes/form.inc on line 1520

# # #

+1 to what Joseph is saying.

I’m doing a late in life D6 to D7, fairly complex, site build for a exceedingly long term client. They’ve used Drupal for over 7 years. I can’t count the number of 5+ year old D7 bugs we’ve hit, and they’re ready to ditch Drupal entirely.

I know every developer wants to play with the shiniest, new toy, but how many people does Drupal want to force to other CMSes because Drupal won’t, or can’t be bothered to, fix the current ‘production’ version?

While we shouldn’t, do we need a special Issue tag for visibility?

D7 bugs 4+ years old

Regards.

Michael-IDA’s picture

Issue tags: -drupal 7

removing inadvertently added, +drupal 7

cilefen’s picture

Where is the regression test asked for in #29? There is none in the #7 patch.

(Edited)

xjm’s picture

Priority: Critical » Major

As per #66, this issue has been confirmed as a major priority bug, not critical. You can help this fix be committed to both Drupal 8 and Drupal 7 (which are both production versions of Drupal, by the way) by adding a regression test for the bug to the patch. Thanks!

Michael-IDA’s picture

No. D8 is far from 'real world' production use, no matter what D.O tells itself...

Sure, it can make a blog, but can it be a full commerce site with user account balances, automated invoicing, ...?

No offence to anyone working on D8, but reality is reality...

xjm’s picture

@Michael-IDA, if you'd like to help get this bug fixed, please minimize off-topic discussion to avoid distracting from the bugfix itself, and help with the regression test so it can be committed to both D8 and D7. A regression test is needed before commit to ensure the bug is not reintroduced. Drupal core is still an open source project built largely by volunteers, so you can help resolve the issue by contributing.

joseph.olstad’s picture

Between @Michael-IDA , myself, pacow and Brad.D we've spent probably 70 hours in the past two months figuring out how to fix this issue despite the fact that we didn't know about a working core patch that should have been committed 4 years ago. Luckily @Michael-IDA is a very determined and outspoken fellow because if he wasn't he wouldn't have gotten my attention to the severity of this issue and I would not have been motivated enough to figure out what the heck was happening.

So Kudos to @Michael-IDA , he's got every right to be irritated and I thank him for putting a firecracker up my butt and lighting it off , because now I'm just as fired up as he is.

How many >sane< people have just given up? All you have to do is look at the usage stats of Drupal, which is on the downward trend since at least a year or two and it started a couple years before that when all these themers stopped writing great themes for Drupal. I tie a direct co-relation of the downward trend of Drupal usage with the lackluster availability of quality functional contrib themes. How much more important of an issue can there be?

There's a lot of themers that did glorious work on their themes back in 2011/2012 but the barriers to adoption are too high because A) they don't know that they need a patch for whatever reason , for B) they don't know how to patch, or C) they got frustrated with A and B and switched to wordpress.

BTW, this is a CRITICAL issue in my books.

joseph.olstad’s picture

How could we write a relevant core test for this?

managed_file fields only occur when using something like file_entity that provides fieldable files.

Here's my opinion, a test is not required because the patch / solution doesn't cause a regression, it's an improvement to core and passes all existing core tests AND it fixes the contrib issue. Therefore , put it into the dev branch of core and let us trigger contrib tests with the contrib modules to see if there's any regressions (which I doubt there would be given the simplicity of the patch and improved code).

Sometimes you have to think outside the box a bit. Push this into core dev branches, let us trigger the contrib tests and we can compare contrib test results between the latest tagged release test and the latest dev.

! This is an URGENT issue, we simply cannot afford to wait another 4 years for someone to write a test that no one knows how to write.

***EDIT***
AND, 4 years, and no reported regression that hasn't been resolved. Proof is proof is proof.
***EDIT***

Michael-IDA’s picture

Hi Jess, (@xjm)

Yes, Drupal is an open source project built largely by volunteers. If you’re going down that route, I’ve been volunteering my time and effort, in areas I’m well versed in, for near 10 years. You might also notice I did actively contribute to this bug by doing the housekeeping that usually gets ignored. And unlike working for Acquia, people like myself do not get paid to work on the Drupal code base. Anything, and everything, we do on D.O, groups, or forum is un-paid for voluntarism.

To put that into perspective, I’ve personally spent at least 70 hours on testing, documentation, and QA for this single issue. Hard numbers that’s a minimum $5,600 out of my own pocket. My loss of $6K for, again, a D7 bug known about for 5 years. Please allow me to bitch some, thanks...

Now to specifics. You ask me to write a “regression test.” First, I’ve never written one, but I’m sure I can. Second did you mean SimpleTest [1]? If you meant SimpleTest, are you referring to a Functional test, Unit test, or Upgrade test? Do you have any sort of link to a complete example of what you want? Better yet, give me a link to a well written “regression test” (that needs a contrib module to trigger) and the issue it came from.

[Ah, Joseph beat me to it...]

# # #

Oh, yes, testing results, duh, why I came back here today…

Failed: Specific instance, Media module, upgrade path 1.x to 2.x.
Results Summary: Shuts errors up, unknown if it solves complete issue.

See: https://www.drupal.org/node/2884164#comment-12203681

Best,
Michael

[1] https://www.drupal.org/docs/7/testing

xjm’s picture

If it's urgent for you, run a patch on your site with drush make or the like. If you want to help fix Drupal permanently, then help with the test. A test is required because Drupal, like most software, uses test-driven development. Our testing requirements are documented here: https://www.drupal.org/core/gates#testing

@oriol_e9g started providing a test in earlier comments, but for some reason the more recent patches do not include it. @oriol_e9g is on the right track; the correct test will provide a test theme that triggers the bug. Take @oriol_e9g's patch in #58, combine it with @leopathu's fix in #62, and see if the test then passes. If it doesn't, work from there.

xjm’s picture

@Michael-IDA, thanks! I crossposted with you I think. Hopefully #79 will help you get started. Also see: https://www.drupal.org/contributor-tasks/write-tests

joseph.olstad’s picture

joseph.olstad’s picture

Status: Needs work » Needs review
Michael-IDA’s picture

Drupal 8 involving a series of "gates" ...

Well, that leaves me out. I don't do D8 as I'm a production shop only.

FWIW:

Latest testing with just core patch:

wget https://www.drupal.org/files/theme-settings-in-build-info-1862892-7.patch

has good results.

Theme works, doesn’t WSOD, saves, and re-populates from theme’s plain form Browse button (‘Background image URL’) correctly on page re-load.

RTBC patch #7

Best,
Michael

Ref: https://www.drupal.org/node/2884164#comment-12203722

Edit: Or I'll wait a day to re-test whatever patch comes out next...

Status: Needs review » Needs work

The last submitted patch, 81: d8-theme_settings_in_build_info_patch62fix_with_32fortests-1862892-81.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

larowlan’s picture

  1. +++ b/core/modules/system/src/Form/ThemeSettingsForm.php
    @@ -324,9 +324,14 @@ public function buildForm(array $form, FormStateInterface $form_state, $theme =
    +        $theme_settings_file = drupal_get_path('theme', $theme) . '/theme-settings.php';
    +        $theme_file = drupal_get_path('theme', $theme) . '/' . $theme . '.theme';
    

    We could store the result of drupal_get_path in a variable here, instead of calling it twice

  2. +++ b/core/modules/system/src/Tests/Theme/ThemeTestCustomSettings.php
    @@ -0,0 +1,111 @@
    +class ThemeTestCustomSettings extends WebTestBase {
    

    We're trying to avoid adding new tests that extend from the legacy test base

  3. +++ b/core/modules/system/src/Tests/Theme/ThemeTestCustomSettings.php
    @@ -0,0 +1,111 @@
    +    foreach ($themes AS $theme) {
    

    this needs to be as not AS

  4. +++ b/core/modules/system/tests/themes/test_theme/test_theme.theme
    @@ -154,3 +154,61 @@ function test_theme_preprocess_theme_test_preprocess_suggestions__kitten__flamin
    +function test_theme_form_system_theme_settings_alter(&$form, \Drupal\Core\Form\FormStateInterface $form_state) {
    ...
    +function test_theme_form_system_theme_settings_submit(&$form, \Drupal\Core\Form\FormStateInterface $form_state) {
    ...
    +      $file = \Drupal\file\Entity\File::load($element[0]);
    
    +++ b/core/modules/system/tests/themes/test_theme_alter/theme-settings.php
    @@ -0,0 +1,59 @@
    +function test_theme_alter_form_system_theme_settings_alter(&$form, \Drupal\Core\Form\FormStateInterface $form_state) {
    ...
    +function test_theme_alter_form_system_theme_settings_submit(&$form, \Drupal\Core\Form\FormStateInterface $form_state) {
    ...
    +      $file = \Drupal\file\Entity\File::load($element[0]);
    

    Can we import the FQCN with a use statement

  5. +++ b/core/modules/system/tests/themes/test_theme/test_theme.theme
    @@ -154,3 +154,61 @@ function test_theme_preprocess_theme_test_preprocess_suggestions__kitten__flamin
    +    $element = $form_state->getValue('custom_logo');
    +    if (!empty($element[0])){
    

    We could use ->getValue(['custom_logo', '0']) here instead, e.g.

    if ($file_id = $form_state->getValue(['custom_logo', '0'])) {
    // ...
    }
  6. +++ b/core/modules/system/tests/themes/test_theme_alter/test_theme_alter.theme
    @@ -0,0 +1,12 @@
    +<?php
    +
    +/**
    + * @file
    + * Add hooks for tests to use.
    + */
    +
    +/**
    + * Implements hook_preprocess_HOOK() for theme_test_template_test templates.
    + */
    +function test_subsubtheme_preprocess_theme_test_template_test(&$variables) {
    +}
    

    Not needed

  7. +++ b/core/modules/system/tests/themes/test_theme_alter/theme-settings.php
    @@ -0,0 +1,59 @@
    + * Implements hook_form_system_theme_settings_alter().
    

    Instead of duplicating the logic here, we could use require_once to load the other .theme file and then call the other method?

xjm’s picture

As a note, the core gates have been in use for six or seven years (so, longer than this issue has been open) and also apply to Drupal 7.

joseph.olstad’s picture

I've implemented most of the suggestions made by Larowlan in #85

However, the real remaining issue from reading the thread and looking at the test results is related to AJAX (search for AJAX in this issue and look at the test results for AJAX or ajax.

seems like for quite a while no one has been able to figure the ajax challenge left.

Here's a new (hopefully improved) patch with interdiff to show the difference between patch 81 and patch 87

Testing versus 8.3.x this time , instead of 8.5.x

joseph.olstad’s picture

small oops, I will upload a new patch

Status: Needs review » Needs work
joseph.olstad’s picture

joseph.olstad’s picture

This is about as far as I can take the D8 patch, if someone could please pick up the ball and drive this into the end-zone it'd be very much appreciated.
I haven't yet used D8 and I'm not familiar with it other than installing it on simplytest.me a couple times and pressing a few buttons.

Status: Needs review » Needs work
larowlan’s picture

@joseph.olstad can you add a 'Remaning tasks' section to the issue description listing what still needs to be addressed?

Thanks

joseph.olstad’s picture

Sure, I will update the remaining tasks and also fix the missing ; in the latest patch

joseph.olstad’s picture

Issue summary: View changes
Status: Needs work » Needs review
xjm’s picture

Thanks @joseph.olstad! Looking at the results for that test in https://www.drupal.org/pift-ci-job/733026:

fail: [Other] Line 82 of core/modules/system/src/Tests/Theme/ThemeTestCustomSettings.php:
Ajax response header found.
Value false is identical to value '1'.

fail: [Browser] Line 106 of core/modules/system/src/Tests/Theme/ThemeTestCustomSettings.php:
Found field with name 'files[custom_logo]' and value '/var/www/html/sites/simpletest/43258707/files/image-test.png'

fail: [Other] Line 82 of core/modules/system/src/Tests/Theme/ThemeTestCustomSettings.php:
Ajax response header found.
Value false is identical to value '1'.

That's here in the patch:

+++ b/core/modules/system/src/Tests/Theme/ThemeTestCustomSettings.php
@@ -0,0 +1,111 @@
+      // Add a new managed file.
+      $image_file = $this->getTestFile('image');
+      $edit1['files[custom_logo]'] = drupal_realpath($image_file->getFileUri());
+      $this->drupalPostAjaxForm(NULL, $edit1, 'custom_logo_upload_button');

So the reason the test is failing is that the field doesn't exist on the page (at least as labeled there) within the test. Running the test locally and inspecting the verbose HTML output would be a good way to debug that further.

Status: Needs review » Needs work

The last submitted patch, 95: d8-theme_settings_in_build_info_patch62fix_with_32fortests_IMPROVED-1862892-95.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

joseph.olstad’s picture

larowlan, would part of the test fail be related to your 5th recommendation?

I wonder if we should revert the 5th recommendation by larowlan? not sure.
https://www.drupal.org/node/1862892#comment-12203776

anyhow, I made the recommended changes (all except one)
Hoping someone else can guide this now. I'm going to open a D7 issue to continue working on the D7 patch as that's the reason why I'm working on this issue.

Thanks

larowlan’s picture

+++ b/core/modules/system/tests/themes/test_theme/test_theme.theme
@@ -201,12 +204,12 @@ function test_theme_form_system_theme_settings_alter(&$form, \Drupal\Core\Form\F
+  if ($form_state->getValue(['custom_logo', '0'])) {
+    $element = $form_state->getValue(['custom_logo', '0']);
     if (!empty($element[0])){
       // Make submited files permanent.
-      $file = \Drupal\file\Entity\File::load($element[0]);
+      $file = File::load($element[0]);

Ya, you need to do this


if ($file_id = $form_state->getValue(['custom_logo', '0'])) {
  $file = File::load($file_id);
  // ...

oriol_e9g’s picture

In #58 I tried to make a green test with the fix and failed... try again with more help :)

Fixing some errors.

larowlan’s picture

Status: Needs work » Needs review

Getting very close now

  1. +++ b/core/modules/system/src/Tests/Theme/ThemeTestCustomSettings.php
    @@ -59,7 +59,8 @@ protected function getLastFileId() {
    -   * Tests extended theme settings with custom fields and custom submit handlers.
    ...
    +   * handlers.
    

    This has to be <80, so can you rephrase the description to something like

    'Tests extended theme settings with custom fields and submit handlers.'

  2. +++ b/core/modules/system/tests/themes/test_theme/test_theme.theme
    @@ -205,13 +205,10 @@ function test_theme_form_system_theme_settings_alter(&$form, FormStateInterface
    +    // Make submited files permanent.
    
    +++ b/core/modules/system/tests/themes/test_theme_alter/theme-settings.php
    @@ -47,13 +47,10 @@ function test_theme_alter_form_system_theme_settings_alter(&$form, \Drupal\Core\
    +    // Make submited files permanent.
    

    Submitted should have two t's

oriol_e9g’s picture

Version: 8.3.x-dev » 8.4.x-dev
Assigned: Unassigned » oriol_e9g

OK, working on this and the last 3 fails.

oriol_e9g’s picture

The problem is that drupalPostAjaxForm cannot fire the submit managed_file button.

You can see: <label for="edit-custom-logo-upload"> but doesn't exists the name="edit-custom-logo-upload" and you can see duplicated ID id="edit-custom-logo-upload".

And if I fire the submit with name="custom_logo_upload_button" the file is not attached.

How can I fired the ajax form and attach the file in tests? Nothing seems to work.

<div id="ajax-wrapper">
<div class="js-form-item form-item js-form-type-managed-file form-type-managed-file js-form-item-custom-logo form-item-custom-logo">
<label for="edit-custom-logo-upload">Secondary logo.</label>
<div id="edit-custom-logo-upload" class="js-form-managed-file form-managed-file">
<div id="ajax-wrapper"><input data-drupal-selector="edit-custom-logo-upload" type="file" id="edit-custom-logo-upload" name="files[custom_logo]" size="22" class="js-form-file form-file">
<input class="js-hide button js-form-submit form-submit" data-drupal-selector="edit-custom-logo-upload-button" formnovalidate="formnovalidate" type="submit" id="edit-custom-logo-upload-button" name="custom_logo_upload_button" value="Upload">
<input data-drupal-selector="edit-custom-logo-fids" type="hidden" name="custom_logo[fids]">
</div>
</div>
</div>
</div>

Status: Needs review » Needs work

The last submitted patch, 104: 1862892.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

joseph.olstad’s picture

some minor changes, see interdiff. expect same result. ?

joseph.olstad’s picture

nice work oriol_e9g , so are you saying that if we combine #2346893 and #2497909 we should then get a pass ? so
what is the strategy, roll them into this? or work on those seperately? I think it might go faster if we roll those in if they're not too big. Could a core maintainer just picked up and run with this? punt it into the end-zone like a good all-star kicker at the superbowl

joseph.olstad’s picture

joseph.olstad’s picture

oriol_e9g , please review the issue summary change I made for 'Remaining Tasks' and update it if you need to, not sure which issue is left is it #2346893: or #2497909 , or both issues?

joseph.olstad’s picture

Status: Needs review » Needs work

The last submitted patch, 107: 1862892-107.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

joseph.olstad’s picture

Status: Needs work » Needs review
FileSize
673 bytes
12.27 KB

new patch, undo change to core/modules/system/tests/themes/test_theme_alter/theme-settings.php
remove: use Drupal\Core\Form\FormStateInterface;

joseph.olstad’s picture

FileSize
756 bytes
12.27 KB

nitpick

joseph.olstad’s picture

oriol_e9g’s picture

oriol_e9g’s picture

Ups! @joseph.olstad I have posted #116 before seen your last posts.

I haved refactored the tests and I think that now we have a functional green tests.

We do not really need # 2346893 and # 2497909 to fix it, but they are necessary if we want a correct HTML output for file_managed form.

The last submitted patch, , failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

The last submitted patch, , failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

The last submitted patch, , failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

joseph.olstad’s picture

Awesome work by oriol_e9g, just trying to help out here.
121 includes only changes as recommended by code sniffer.
Rolling a new test.

joseph.olstad’s picture

FileSize
953 bytes
12.16 KB

code sniffer still had two complaints, deal with those two here in patch 122

xjm’s picture

Nice work @oriol_e9g and @joseph.olstad!

Since it's now passing and cleaned up, can we upload this or the next version with a test-only patch to expose and validate the test coverage for the bug? (So it would include everything in the current patch except the fix in core/modules/system/src/Form/ThemeSettingsForm.php.)

joseph.olstad’s picture

tests only patch as requested, expect failure to prove that patch 122 is fixing a core bug.

joseph.olstad’s picture

meanwhile, can someone please start the backport of the D8 tests from #1862892-124: When a theme has a managed_file field and a submit callback, call to undefined function error is thrown.

work on the backport needs to be done in this issue:
#2900373: [D7] When a theme has a managed_file field and a submit callback, call to undefined function error is thrown.

If someone could please start the backport I could help finish it. Really would appreciate the assistance as this is a very important issue that deserves our immediate attention.

We just cannot wait any longer.

Thanks

joseph.olstad’s picture

joseph.olstad’s picture

Status: Needs review » Needs work

Why is our tests only patch passing? I thought the whole point of this was to test what we're trying to fix?

joseph.olstad’s picture

maybe it is as I suspected from the beginning, beyond core scope to test this scenario because (at least in D7 , not sure about D8) it requires contrib to be able to make managed files (using the file_entity module).

So, if this is true, we commit the fix (because we know we need it) and the added tests give us insurance / assurance that it IS working and that our added tests will ensure that future commits will not break this functionality worse than it already is prior to this fix.

joseph.olstad’s picture

larowlan’s picture

Status: Needs review » Needs work

The test should have failed, therefore it doesn't provide proof that the bug exists.

If the bug only exists when a specific contrib module is enabled, either the bug is in that module - or - we need a test module that is enabled in the test that replicates the scenario in which the bug exists.

larowlan’s picture

+++ b/core/modules/system/src/Tests/Theme/ThemeTestCustomSettings.php
@@ -0,0 +1,121 @@
+namespace Drupal\system\Tests\Theme;
...
+class ThemeTestCustomSettings extends BrowserTestBase {

This is the wrong namespace for a BrowserTestBase.

That may be why it didn't fail, because it may not even be running.

You need to make the namespace

\Drupal\Tests\system\Functional\Theme

And the class needs to live in

core/modules/system/tests/src/Functional/Theme/ThemeTestCustomSettings.php

joseph.olstad’s picture

Status: Needs work » Needs review
FileSize
536 bytes
11.05 KB
12.2 KB

It's getting late here in Eastern Standard Time, but here it is:
Changes to the patch as recommended by @larowlan

includes interdiff from 122 to 132
includes tests only patch (no fix expect a fail)
includes fix with tests patch (expect success)

joseph.olstad’s picture

@oriol_e9g or @larowlan,

Are we 100% sure that D8 is affected by this issue?

I've only confirmed that it's a D7 issue myself.

In the test code would it be possible to add an assert in:
test_theme_form_system_theme_settings_alter

the assert would just assert that it's getting called.

also, another assert in
test_theme_form_system_theme_settings_submit

the reasoning for this is that we need to know whether test_theme_form_system_theme_settings_submit is getting called or not. If it IS getting called, then either A) D8 core is not affected , or B) we have missed something in the test scenario

I'd be happy with a Closed 'Works as designed' for this issue so that we can get on with fixing it in D7.

oriol_e9g’s picture

Yes, I have this problem with a custom template in Drupal 8.3. What am i missing? tests always pass?

oriol_e9g’s picture

Status: Needs review » Needs work

The last submitted patch, 135: wtf.patch, failed testing. View results

xjm’s picture

Issue tags: -D7 bugs 4+ years old +Needs issue summary update

Well, we know the test did run in https://www.drupal.org/pift-ci-job/733659 because it failed there.

And yeah, as @larowlan says, when we have a bug that contrib exposes but that needs a fix in core, we add test modules and themes that replicate the part of the contrib module that exposes the bug. That's how we ensure we don't reintroduce the issue in the future (thence "regression test").

In my experience, the best way to debug a test that's not failing as it should is to run it locally and examine the verbose output. You can even install the setup the test and actually click around to see where it's different from your test site.

I didn't understand from the summary that "Managed File field" was a field provided by a contributed module (file_managed is also the name of a core DB table and I read it as a reference to private files). If it were an issue only with that module, then the fix might belong in that module, but @oriol_e9g has reproduced the bug in D8. @oriol_e9g, can you confirm that you don't have a D8 port of https://www.drupal.org/project/managed_file or something like it on the test site? Is just your theme & the testing profile or minimal etc. sufficient to reproduce the bug? Tagging for a summary update with that info.

It's weird and brittle that a form submit handler is including files in the first place. Quite a lot of logic for a submit handler.

xjm’s picture

joseph.olstad’s picture

I'd rather that this bug was fixed in core because as a contrib maintainer I don't want to have to write a theme alter hook to implement the documented workaround to support themes that when a said contrib module is enabled the theme crashes. We know that the core patch doesn't break anything and is very simple. Themers are a very valuable resource for Drupal and we should try to make their lives easier and not throw them in the ditch over some policy wonking.

joseph.olstad’s picture

Can this issue please get escalated?

xjm’s picture

@joseph.olstad, there isn't anyone to whom we can "escalate" the issue. It's just us, the contributors on the issue. The issue has already received responses from two release managers and a framework manager recently, as well as review by all the theme system maintainers and framework managers a few months ago. Let's just assume that anyone who has taken the time to respond on this issue cares about others' theming experience and leave it at that. Thanks!

It's a major bug that needs tests and a robust fix. If we want the fix in core, it's especially important that the core test suite include the conditions that prove the bug so we can ensure future theme system changes do not further break this functionality. This is especially important because this form submit handler is a little brittle in D7 and D8 already. We are close to having a test that reproduces the bug -- we just need to do some debugging, possibly in the test environment, and find out what the difference is. This is very valuable information to ensure the regression is not reintroduced.

I'd suggest @oriol_e9g do the debugging since they have the real-world D8 site that surfaces the bug. That's valuable information to understand what is happening and ensure a complete fix. In the meanwhile, applying the patches @joseph.olstad has provided for both D8 and D7 is a helpful hotfix if you have a site affected by this.

oriol_e9g’s picture

I'm busy, maybe I can work on tests next week.

joseph.olstad’s picture

I've set up a D8 environment for local testing on my server but getting complications with composer not working and having to configure PHP UNIT to work with jailkit.

Anonymous’s picture

Status: Needs work » Needs review
FileSize
7.65 KB

Hello. Sorry if my patch violates any of the accepted directions or duplications (there are a lot of posts, and my English is weak).

For quick fix this problem in custom theme:

function THEME_form_system_theme_settings_alter(&$form, &$form_state) {
  # form settings here
  # ...

  $form['#submit'][] = 'THEME_form_system_theme_settings_submit';

  $theme = \Drupal::theme()->getActiveTheme()->getName();
  // Example for theme-settings.php file.
  $theme_file = drupal_get_path('theme', $theme) . '/theme-settings.php';
  
  $build_info = $form_state->getBuildInfo();
  if (!in_array($theme_file, $build_info['files'])) {
    $build_info['files'][] = $theme_file;
  }
  $form_state->setBuildInfo($build_info);
}

Looks like the problem arises when the form is cached here:

$check_cache = isset($input['form_id']) && $input['form_id'] == $form_id && !empty($input['form_build_id']);
    if ($check_cache) {
      $form = $this->getCache($input['form_build_id'], $form_state);
    }

    // If the previous bit of code didn't result in a populated $form object, we
    // are hitting the form for the first time and we need to build it from
    // scratch.
    if (!isset($form)) {

Self-review:

  1. +++ b/core/modules/system/tests/src/FunctionalJavascript/ThemeFormSettingsTest.php
    @@ -0,0 +1,78 @@
    +class ThemeFormSettingsTest extends JavascriptTestBase {
    

    For a more realistic imitation of what is happening.

  2. +++ b/core/modules/system/tests/src/FunctionalJavascript/ThemeFormSettingsTest.php
    @@ -0,0 +1,78 @@
    +  /**
    +   * Provides test data for ::testFormSettingsSubmissionHandler().
    +   */
    +  public function providerTestFormSettingsSubmissionHandler() {
    +    return [
    +      'test theme.theme' => ['test_theme_theme'],
    +      'test theme-settings.php' => ['test_theme_settings'],
    +    ];
    +  }
    

    I refused to use the theme "test_theme" because it has an extremely unfriendly library configuration (see test_theme.info.yml file). And changed theme names. But perhaps this is still not the best name.

  3. +++ b/core/modules/system/tests/themes/test_theme_settings/test_theme_settings.info.yml
    @@ -0,0 +1,6 @@
    +base theme: false
    
    +++ b/core/modules/system/tests/themes/test_theme_theme/test_theme_theme.info.yml
    @@ -0,0 +1,6 @@
    +base theme: false
    

    If we not use 'test_basetheme', we not need change ThemeTest.php

  4. +++ b/core/modules/system/tests/themes/test_theme_theme/test_theme_theme.theme
    @@ -0,0 +1,36 @@
    +function test_theme_theme_form_system_theme_settings_alter(&$form, FormStateInterface $form_state) {
    ...
    +  $form['custom_logo'] = [
    

    Only 'custom_logo', because while we not test any other fields.

Anonymous’s picture

+++ b/core/modules/system/src/Form/ThemeSettingsForm.php
@@ -331,7 +331,13 @@ public function buildForm(array $form, FormStateInterface $form_state, $theme =
-            $form_state->addBuildInfo('files', [$filename]);
+
+            // The file must be require for cached form too.
+            $files = $form_state->getBuildInfo()['files'];
+            if (!in_array($filename, $files)) {
+              $files[] = $filename;
+            }
+            $form_state->addBuildInfo('files', $files);

Not just $form_state->addBuildInfo('files', [$filename]), because this method rigidly replaces 'files' key. So we can lost existing data here.

The last submitted patch, 144: 1862892-144-test-only.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

joseph.olstad’s picture

Nice work @vaplas, we are very close to RTBC, all that is left is to make the recommended code sniffer changes outlined in comment #146 #1862892-146: When a theme has a managed_file field and a submit callback, call to undefined function error is thrown.

Not only does the new patch prove that core needs the fix, it is less code. So great work. Let us know if you want help with the code sniffer recommended changes as mentioned by the testbot in comment #146.

If you can, please backport the tests to D7 there is a related D7 issue.
#2900373: [D7] When a theme has a managed_file field and a submit callback, call to undefined function error is thrown.
You could very soon be soon rich and famous ! Many thanks in advance !

Anonymous’s picture

@joseph.olstad, thank your for review and such positive feedback!

Coding standards fixed (returned file descriptions from #132, sorry for the removal of this).

Unfortunately, I'm noob with d7, so I can only help with d8.

joseph.olstad’s picture

Status: Needs review » Reviewed & tested by the community

Great work @vaplas and @oriol_e9g and everyone.
RTBC #148 as-is.
Get this nasty bug fixed once and for all! Please commit!
there's no denying that core has a bug, the tests prove it and the test also proves that the fix works.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 148: 1862892-148.patch, failed testing. View results

joseph.olstad’s picture

Version: 8.4.x-dev » 8.3.x-dev
Status: Needs work » Reviewed & tested by the community

some unrelated instability in 8.4.x
RTBC and holding

joseph.olstad’s picture

Version: 8.3.x-dev » 8.5.x-dev
joseph.olstad’s picture

Issue summary: View changes
Issue tags: -Needs tests
joseph.olstad’s picture

joseph.olstad’s picture

We've done as requested. As far as I can tell, this is ready for commit. Any chance we can get this in 8.5.x? If you do not want to commit to 8.5.x please justify with a constructive explanation.

larowlan’s picture

Status: Reviewed & tested by the community » Needs review

Thanks, the final patch looks ready to me - but can we have two patches - one with just the test (that should fail) and one with both the test and the fixes (which will pass). Just to demonstrate that the test actually surfaces the error.

Thanks

Updating issue credit to add the following

  • xjm for mentoring
  • myself for review
Anonymous’s picture

Thanks @joseph.olstad, @larowlan, @xjm and everyone, who invested his labor and time in this issue! I divided #148 into three parts, according to #157. Also update IS to better match the D8.

The last submitted patch, 158: 1862892-158-test-only.patch, failed testing. View results

cilefen’s picture

Status: Needs review » Reviewed & tested by the community

I am putting this back to RTBC because what was asked for in #157 was done.

cilefen’s picture

Status: Reviewed & tested by the community » Needs work

Whoops - I read all comments usually but I was trying to get this done sooner.

  1. +++ b/core/modules/system/src/Form/ThemeSettingsForm.php
    @@ -324,9 +324,21 @@ public function buildForm(array $form, FormStateInterface $form_state, $theme =
    +            // The file must be require for cached form too.
    

    I think this comment contains a typo.

+++ b/core/modules/system/tests/themes/test_theme_settings/theme-settings.php
@@ -0,0 +1,40 @@
+  // Add a new submission handler.
+  $form['#submit'][] = 'test_theme_settings_form_system_theme_settings_submit';

+++ b/core/modules/system/tests/themes/test_theme_theme/test_theme_theme.theme
@@ -0,0 +1,41 @@
+  // Add a new submission handler.
+  $form['#submit'][] = 'test_theme_theme_form_system_theme_settings_submit';

Comments such as these can be superflous. They're not wrong, but they are simply restating what the code is doing. But what the code is doing is clear. So now they are one more thing to maintain.

cilefen’s picture

Anonymous’s picture

#162: thank you, @cilefen! Your edit is absolutely correct. I'm sorry that it happened (in russian language both forms of this word are appropriate for conveying this idea).

joseph.olstad’s picture

Status: Needs review » Reviewed & tested by the community

You'd need a magnifying glass now to see a flaw.

  • larowlan committed 7f9287e on 8.5.x
    Issue #1862892 by oriol_e9g, joseph.olstad, vaplas, G.I.Joe, leopathu,...

  • larowlan committed 6f04c4a on 8.4.x
    Issue #1862892 by oriol_e9g, joseph.olstad, vaplas, G.I.Joe, leopathu,...
larowlan’s picture

Version: 8.5.x-dev » 7.x-dev
Status: Reviewed & tested by the community » Patch (to be ported)

Committed as 7f9287e and pushed to 8.5.x

Cherry-picked as 6f04c4a and pushed to 8.4.x

Ready for D7 backport now.

joseph.olstad’s picture

Version: 7.x-dev » 8.5.x-dev
Status: Patch (to be ported) » Fixed

Thanks for the work on this everyone.
D7 backport in another ticket to avoid spamming D8 team.
see
For those that wish to help the D7 backport, please see this issue here:
#2900373: [D7] When a theme has a managed_file field and a submit callback, call to undefined function error is thrown.

we need to backport the D8 tests only patch (above) from comment #158 #1862892-158: When a theme has a managed_file field and a submit callback, call to undefined function error is thrown.

I will update
#2900373: [D7] When a theme has a managed_file field and a submit callback, call to undefined function error is thrown.

Anonymous’s picture

Woot! 🎉

Should we create a CR about this change? Some themes already use a workaround:

http://cgit.drupalcode.org/aegan/tree/aegan.theme#n253
http://cgit.drupalcode.org/business/tree/business.theme#n154
http://cgit.drupalcode.org/druppio_small_business/tree/theme-settings.ph...
http://cgit.drupalcode.org/jethro/tree/jethro.theme#n215
http://cgit.drupalcode.org/space/tree/theme-settings.php#n258

Since 8.4 (thanks @larowlan for cherry-picked, and @joseph.olstad for supporting) this workaround is no longer needed (although nothing will break).

Status: Fixed » Closed (fixed)

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

David_Rothstein’s picture

Note: In the Drupal 7 backport I proposed a simpler automated test that just sets the form to be cacheable directly and then makes sure it can be correctly saved. It doesn't need to deal with managed_file stuff at all. Perhaps it is worth forward-porting this simpler approach.

joseph.olstad’s picture

@David_Rothstein, IMHO I think we should not let this D8 issue postpone the D7 fix any longer . The fix is needed and backporting your tests to D8 will not add anything to D8 , it will only delay the fix for D7. The test coverage is already sufficient in D8, let's focus on D7 now please and thanks again for your test coverage :)

pedrosp’s picture

Following https://www.drupal.org/node/2843391 that refer to this patch.

For us shamelessly running still on Drupal 7, what is the correct patch ?
I see that #26 is the last one tagged as D7.

Btw I have used by mistake the #162 D8 tagged on my D7.64 and I had only the first diff rejected related to /core/modules/system/src/Form/ThemeSettingsForm.php
but other changes are marked as patched.
EDIT: however it doesn't work and just add some files to the root.

joseph.olstad’s picture

@pedrosp , this was fixed a while back, there is no need to patch if you're using the latest release of core, the fix is in both D8 and D7. The fix was included in 7.61 and all subsequent releases. So if you are using 7.64 it is included.

here is the D7 issue
#2900373: [D7] When a theme has a managed_file field and a submit callback, call to undefined function error is thrown.