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:
Needs testsDone!- Commit patch #1862892-148: When a theme has a managed_file field and a submit callback, call to undefined function error is thrown.
Comment | File | Size | Author |
---|---|---|---|
#162 | 1862892-162.patch | 9.15 KB | cilefen |
| |||
#162 | 1862892-interdiff-158.txt | 1.85 KB | cilefen |
#158 | interdiff-158-fix-only.txt | 1.36 KB | Anonymous (not verified) |
#158 | 1862892-158.patch | 9.17 KB | Anonymous (not verified) |
#158 | 1862892-158-test-only.patch | 7.81 KB | Anonymous (not verified) |
Comments
Comment #1
apadernoComment #2
alisamar CreditAttribution: alisamar commentedvalidation functions also seems to be undefined.
Comment #3
apadernoComment #4
pschuelke CreditAttribution: pschuelke commentedI'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.
Comment #5
peterpoe CreditAttribution: peterpoe commentedI have this long (but not too ugly) workaround. Just place the following in a custom module:
Comment #6
peterpoe CreditAttribution: peterpoe commentedThe 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):
Comment #7
peterpoe CreditAttribution: peterpoe commentedUpdated patch with correct path to file(s).
Comment #8
joaogarin CreditAttribution: joaogarin commentedHello,
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
Comment #9
pdcarto CreditAttribution: pdcarto commentedThis seems to be working for me inside my theme's MYTHEME_form_system_theme_settings_alter function in my theme-settings.php:
Comment #10
aaronschachter CreditAttribution: aaronschachter commentedWe 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)
Comment #11
Talkless CreditAttribution: Talkless commentedThanks for walkaround.
I wonder, how is it with Drupal 8? Maybe there will be simpler way to upload permanent file in theme settings?
Comment #12
robynlgreen CreditAttribution: robynlgreen commentedJust 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
Comment #13
damonbla CreditAttribution: damonbla commentedGood 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?
Comment #14
basant CreditAttribution: basant at Innoraft commentedI 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.
Comment #15
nabajit CreditAttribution: nabajit commentedAdded #9 and #10 in my theme settings which worked like a charm for me.
Thanks all.
Comment #16
deggertsen CreditAttribution: deggertsen as a volunteer commentedThe 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.
Comment #17
leopathu CreditAttribution: leopathu as a volunteer and commentedSame thing happening in Drupal 8 too,
Any solutions ?
Comment #18
G.I.Joe CreditAttribution: G.I.Joe commentedAdded a new patch with simplified code.This patch and interdiff is useless. Was created with a color format.
Comment #20
G.I.Joe CreditAttribution: G.I.Joe commentedAdded a new patch with simplified code.Wrong comment_number.
Comment #21
G.I.Joe CreditAttribution: G.I.Joe commentedAdded a new patch with simplified code.
Comment #22
G.I.Joe CreditAttribution: G.I.Joe commentedComment #23
CProfessionals CreditAttribution: CProfessionals commentedI had a similar problem with Corporate3X Theme. I applied #9. Just to be a little clearer after pasting the code:
Hope this helps.
Thanks for figuring this out!
Comment #24
SchwebDesign CreditAttribution: SchwebDesign commentedtested #21 but it gave error:
changing line 565
from
to
seemed to fix that error
and then this patch worked for me
Comment #25
danquah CreditAttribution: danquah at Reload commentedWhile 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.
Comment #26
G.I.Joe CreditAttribution: G.I.Joe as a volunteer commentedgood point. Created new patch. please, review.ok never mind. :-)
Comment #28
G.I.Joe CreditAttribution: G.I.Joe as a volunteer commentedComment #29
Fabianx CreditAttribution: Fabianx as a volunteer and at Tag1 Consulting commented+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.
Comment #30
David_Rothstein CreditAttribution: David_Rothstein as a volunteer commentedComment #31
leopathu CreditAttribution: leopathu as a volunteer and commentedIf 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 ?
Comment #32
leopathu CreditAttribution: leopathu commentedI hope, This patch would fix the issue in drupal 8, But Needs to be more discuss on comment #31
Comment #33
leopathu CreditAttribution: leopathu commentedComment #35
joelpittetIs this needed still? I expect
getPath()
works fine. The real fix is the other line.Anybody interested in writing a test for this?
Comment #36
joelpittetfor #35
Comment #37
Fabianx CreditAttribution: Fabianx as a volunteer and at Tag1 Consulting commentedDefinitely 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.
Comment #38
oriol_e9gWe 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.Comment #39
oriol_e9gI'm trying to add some tests, but they are not executing locally... I will appreciate some guidance :S
Comment #40
oriol_e9gComment #42
oriol_e9gComment #44
oriol_e9gMaybe I have to use WebTestBase instead of BrowserTestBase?
Comment #45
joelpittetWebTestBase or KernalTestBase should do the trick. BrowserTestBase is more for JS FWIU
Comment #46
oriol_e9gTests progress, still needs work.
The "managed_file" field it's not appearing in the settings form and I don't kwo why :S
Comment #48
leopathu CreditAttribution: leopathu commentedWhat do you think about the #31 Comment ?
Comment #49
oriol_e9gIf 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
Comment #50
oriol_e9gInitial 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.
Comment #51
oriol_e9gI can not work on it for a while, maybe someone else wants to complete the work in the meantime.
Comment #53
oriol_e9gI 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
Comment #54
oriol_e9gThe same bug it's also related here: https://www.drupal.org/node/2779947
Comment #55
oriol_e9gThe complet test coverage should be something like this tests.
We still need to find a solution for the THEMENAME.theme case.
Comment #57
oriol_e9gThe managed_file test notices are related to #1903010: Notice: Undefined index: #field_name in file_managed_file_save_upload()
Comment #58
oriol_e9gComment #59
leopathu CreditAttribution: leopathu commentedi 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.
Comment #62
leopathu CreditAttribution: leopathu commentedi hope, this patch would pass the test.
Comment #63
oriol_e9gAnybody knows why test always throw this error?
{"message":"A fatal error occurred: The specified #ajax callback is empty or not callable."}
Comment #64
leopathu CreditAttribution: leopathu commentedComment #65
leopathu CreditAttribution: leopathu commentedI think, @oriol_e9g This #63 is happening because of this (handling in this page) issue. but not sure.
Comment #66
xjm@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!
Comment #68
joseph.olstadIf 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
Comment #69
Michael-IDA CreditAttribution: Michael-IDA at Internet Design Alliance commentedHousekeeping. 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.
# # #
+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?
Regards.
Comment #70
Michael-IDA CreditAttribution: Michael-IDA at Internet Design Alliance commentedremoving inadvertently added, +drupal 7
Comment #71
cilefen CreditAttribution: cilefen as a volunteer commentedWhere is the regression test asked for in #29? There is none in the #7 patch.
(Edited)
Comment #73
xjmAs 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!
Comment #74
Michael-IDA CreditAttribution: Michael-IDA at Internet Design Alliance commentedNo. 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...
Comment #75
xjm@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.
Comment #76
joseph.olstadBetween @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.
Comment #77
joseph.olstadHow 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***
Comment #78
Michael-IDA CreditAttribution: Michael-IDA at Internet Design Alliance commentedHi 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
Comment #79
xjmIf 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.
Comment #80
xjm@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
Comment #81
joseph.olstadI've combined patch 62 (passed) with patch 32 (for tests)
Comment #82
joseph.olstadComment #83
Michael-IDA CreditAttribution: Michael-IDA at Internet Design Alliance commentedDrupal 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...
Comment #85
larowlanWe could store the result of drupal_get_path in a variable here, instead of calling it twice
We're trying to avoid adding new tests that extend from the legacy test base
this needs to be
as
notAS
Can we import the FQCN with a use statement
We could use
->getValue(['custom_logo', '0'])
here instead, e.g.Not needed
Instead of duplicating the logic here, we could use require_once to load the other .theme file and then call the other method?
Comment #86
xjmAs 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.
Comment #87
joseph.olstadI'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
Comment #88
joseph.olstadsmall oops, I will upload a new patch
Comment #90
joseph.olstadComment #91
joseph.olstadThis 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.
Comment #93
larowlan@joseph.olstad can you add a 'Remaning tasks' section to the issue description listing what still needs to be addressed?
Thanks
Comment #94
joseph.olstadSure, I will update the remaining tasks and also fix the missing ; in the latest patch
Comment #95
joseph.olstadtry this one, expect 22,112 passes and 3 fails
Comment #96
joseph.olstadComment #97
xjmThanks @joseph.olstad! Looking at the results for that test in https://www.drupal.org/pift-ci-job/733026:
That's here in the patch:
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.
Comment #99
joseph.olstadlarowlan, 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
Comment #100
larowlanYa, you need to do this
Comment #101
oriol_e9gIn #58 I tried to make a green test with the fix and failed... try again with more help :)
Fixing some errors.
Comment #102
larowlanGetting very close now
This has to be <80, so can you rephrase the description to something like
'Tests extended theme settings with custom fields and submit handlers.'
Submitted should have two t's
Comment #103
oriol_e9gOK, working on this and the last 3 fails.
Comment #104
oriol_e9gThe problem is that drupalPostAjaxForm cannot fire the submit managed_file button.
You can see:
<label for="edit-custom-logo-upload">
but doesn't exists thename="edit-custom-logo-upload"
and you can see duplicated IDid="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.
Comment #106
oriol_e9gFor a clenaup and valid HTML file_managed output: #2346893: Duplicate AJAX wrapper around a file field and #2497909: Duplicate HTML IDs are created for file_managed_file fields
Comment #107
joseph.olstadsome minor changes, see interdiff. expect same result. ?
Comment #108
joseph.olstadnice 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
Comment #109
joseph.olstadComment #110
joseph.olstadoriol_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?
Comment #111
joseph.olstadI've created a D7 clone issue for this so that we can line up our ducks in a row.
#2900373: [D7] When a theme has a managed_file field and a submit callback, call to undefined function error is thrown.
Comment #113
joseph.olstadnew patch, undo change to core/modules/system/tests/themes/test_theme_alter/theme-settings.php
remove:
use Drupal\Core\Form\FormStateInterface;
Comment #114
joseph.olstadnitpick
Comment #115
joseph.olstadcombined rollup patch with the ajax fix
Comment #116
oriol_e9gThe previous tests was very bugged so go with this!
* Tests refactored
* Some bugs fixed
* Tests rewrited using BrowserTestBase instead of old Simpletest
* And now... green :)
Comment #117
oriol_e9gUps! @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.
Comment #121
joseph.olstadAwesome work by oriol_e9g, just trying to help out here.
121 includes only changes as recommended by code sniffer.
Rolling a new test.
Comment #122
joseph.olstadcode sniffer still had two complaints, deal with those two here in patch 122
Comment #123
xjmNice 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
.)Comment #124
joseph.olstadtests only patch as requested, expect failure to prove that patch 122 is fixing a core bug.
Comment #125
joseph.olstadmeanwhile, 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
Comment #126
joseph.olstadComment #127
joseph.olstadWhy is our tests only patch passing? I thought the whole point of this was to test what we're trying to fix?
Comment #128
joseph.olstadmaybe 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.
Comment #129
joseph.olstadreview #1862892-128: When a theme has a managed_file field and a submit callback, call to undefined function error is thrown.
Comment #130
larowlanThe 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.
Comment #131
larowlanThis 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
Comment #132
joseph.olstadIt'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)
Comment #133
joseph.olstad@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.
Comment #134
oriol_e9gYes, I have this problem with a custom template in Drupal 8.3. What am i missing? tests always pass?
Comment #135
oriol_e9gComment #137
xjmWell, 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.
Comment #138
xjmComment #139
joseph.olstadI'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.
Comment #140
joseph.olstadCan this issue please get escalated?
Comment #141
xjm@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.
Comment #142
oriol_e9gI'm busy, maybe I can work on tests next week.
Comment #143
joseph.olstadI'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.
Comment #144
Anonymous (not verified) CreditAttribution: Anonymous commentedHello. 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:
Looks like the problem arises when the form is cached here:
Self-review:
For a more realistic imitation of what is happening.
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.
If we not use 'test_basetheme', we not need change ThemeTest.php
Only 'custom_logo', because while we not test any other fields.
Comment #145
Anonymous (not verified) CreditAttribution: Anonymous commentedNot just
$form_state->addBuildInfo('files', [$filename])
, because this method rigidly replaces 'files' key. So we can lost existing data here.Comment #147
joseph.olstadNice 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 !
Comment #148
Anonymous (not verified) CreditAttribution: Anonymous commented@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.
Comment #149
joseph.olstadGreat 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.
Comment #151
joseph.olstadsome unrelated instability in 8.4.x
RTBC and holding
Comment #152
joseph.olstadComment #153
joseph.olstadComment #154
joseph.olstadComment #155
joseph.olstadWe'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.
Comment #156
joseph.olstadComment #157
larowlanThanks, 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
Comment #158
Anonymous (not verified) CreditAttribution: Anonymous commentedThanks @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.
Comment #160
cilefen CreditAttribution: cilefen as a volunteer commentedI am putting this back to RTBC because what was asked for in #157 was done.
Comment #161
cilefen CreditAttribution: cilefen as a volunteer commentedWhoops - I read all comments usually but I was trying to get this done sooner.
I think this comment contains a typo.
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.
Comment #162
cilefen CreditAttribution: cilefen as a volunteer commentedIf I got the comment wrong that means I didn't understand the comment in the first place.
Comment #163
Anonymous (not verified) CreditAttribution: Anonymous commented#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).
Comment #164
joseph.olstadYou'd need a magnifying glass now to see a flaw.
Comment #167
larowlanCommitted as 7f9287e and pushed to 8.5.x
Cherry-picked as 6f04c4a and pushed to 8.4.x
Ready for D7 backport now.
Comment #168
joseph.olstadThanks 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.
Comment #169
Anonymous (not verified) CreditAttribution: Anonymous commentedWoot! 🎉
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).
Comment #171
David_Rothstein CreditAttribution: David_Rothstein as a volunteer commentedNote: 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.
Comment #172
joseph.olstad@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 :)
Comment #173
pedrospFollowing 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.
Comment #174
joseph.olstad@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.