- Issue created by @gge
- 🇩🇪Germany jurgenhaas Gottmadingen
Are there any details about the log message you're reporting here? Something like a stack trace or at least a php file and line number where an error occurs?
- 🇷🇴Romania gge
In /var/log/apache2/error.log is nothing recoreded and Drupal logs only this:
- 🇩🇪Germany jurgenhaas Gottmadingen
Hmm, I've tried, but I can't reproduce the error. The error message comes from
\Drupal\eca_form\HookHandler::inlineEntityFormAfterBuild
which doesn't do anything when the request is an ajax request. But clicking onAdd another item
is exactly that, an ajax request.So, we need additional information to reproduce and analyse that problem.
- 🇩🇪Germany jurgenhaas Gottmadingen
Ah, wait a minute, a cache rebuild helped me to get the error now as well. I can investigate further now.
- 🇩🇪Germany jurgenhaas Gottmadingen
Here is the exact error that occurs:
Drupal\Core\Field\WidgetBase::massageFormValues(): Argument #1 ($values) must be of type array, string given, called in /var/www/html/web/core/lib/Drupal/Core/Field/WidgetBase.php on line 505
$values = $this->massageFormValues($values, $form, $form_state);
At that point, field name is
field_tags
and$values
is_none
.The
$values
is received from$values = NestedArray::getValue($form_state->getValues(), $path, $key_exists);
while path has the following value:$path = array ( [0] => field_basic [1] => 0 [2] => inline_entity_form [3] => field_tags )
It gets to the same code again soon after where path is just
array([0] => field_tags)
, which is an array as expected. So, my assumption is, it processes all this once for the parent form and then for the IEF. The former fails, the latter succeeds. If that assumption is correct, then we need to prevent the processing for the parent form. - 🇩🇪Germany jurgenhaas Gottmadingen
Assumption is NOT correct. I've removed the
field_tags
from the parent form, so it now only gets to that point once with the pathfield_basic / 0 / inline_entity_form / field_tags
. This is the correct path from the parent form to the fieldfield_basic
, the entity reference to basic page and from there down to thefield_tags
which returns a string instead of an array. - 🇩🇪Germany jurgenhaas Gottmadingen
Not sure what we can do about it. Seems like an issue in core since the above methods are massaging the form values badly. The autocomplete field widget has a default value
null
and returns an empty array, which is correct. Whereas checkboxes or select list field widgets have an empty array as default value, but that returns the string_null
, which is incorrect. - 🇩🇪Germany jurgenhaas Gottmadingen
There you go, if I change the cardinality of the
field_tags
from 1 to unlimited, then the error goes away, as the$values = NestedArray::getValue($form_state->getValues(), $path, $key_exists);
then returns an array as expected. Makes me think even more that this is an issue in core. - 🇩🇪Germany jurgenhaas Gottmadingen
Yep, there are 48+ issues in core that are related to
massageFormValues
.For details, see https://www.drupal.org/project/issues/drupal?text=massageFormValues&stat... →
- 🇩🇪Germany mxh Offenburg
The exception message was introduced for being able to identify bugs like this, while preventing that the site stops working. Therefore treating as a bug makes more sense.
- 🇩🇪Germany jurgenhaas Gottmadingen
Agreed, if there's anything we can do about it, let's do it. I was just under the impression there's something wrong in core and I could find a solution or even workaround.
- 🇩🇪Germany jurgenhaas Gottmadingen
I'll mark this as a task, until we know that this is indeed an ECA bug that is actually fixable.
- 🇩🇪Germany mxh Offenburg
It is a bug, as explained in #12. The origin may be somewhere else, but it occurs here and only when ECA is enabled. I currently don't understand the logic of issue type handling, especially regarding found bugs.
- 🇩🇪Germany jurgenhaas Gottmadingen
As I described in #6, core uses
$values = $this->massageFormValues($values, $form, $form_state);
and assumes, that$values
contains an array afterwards. That is a core bug in the first place.The exception captured in ECA only happens because of that, and I am not convinced that we have any influence over that
massageFormValues
method to prevent that from happening. Or am I missing something @mxh? - 🇩🇪Germany mxh Offenburg
The affected code block where the exception is being thrown is currently implemented as the following:
<?php try { $form['#entity'] = clone $entity; /** @var \Drupal\inline_entity_form\InlineFormInterface $handler */ $handler = \Drupal::entityTypeManager()->getHandler($form['#entity']->getEntityTypeId(), 'inline_form'); $handler->entityFormSubmit($form, $form_state); $info = $form['#eca_ief_info']; $form_state->set([ 'eca_ief', $info['parent'], $info['field_name'], $info['delta'], ], $form['#entity']); } catch (\Throwable $t) { \Drupal::logger('eca_form')->error("An error occurred while trying to build an inline entity from submitted form values. This might be a problem in case you use ECA to extend inline entity forms. Please report this to the ECA issue queue to help us improving it."); } ?>
... where the part
$handler->entityFormSubmit($form, $form_state);
is triggering something that will most probably lead to the error. This calling part is a potential problem because it may call other components too early and they may not be ready for that instruction yet. This is due to the complex form building, validation and submission system in general, but at that place we need an entity with the most recent values. As it's a potential problematic call, it was wrapped intentionally with that exception handling. It is ECA that is causing the error. Furthermore, it is a runtime problem and not a problem of static code. - 🇩🇪Germany jurgenhaas Gottmadingen
Yes, I understand that and it's good that we have that shout out mechanic included so that this creates awareness.
In addition to that, can you agree that the following code is a bug as
\Drupal\Core\Field\WidgetInterface::massageFormValues
restricts$values
to be an array while\Drupal\Component\Utility\NestedArray::getValue
returns mixed types and nothing ensure values to be an array?$values = NestedArray::getValue($form_state->getValues(), $path, $key_exists); $values = $this->massageFormValues($values, $form, $form_state);
- 🇩🇪Germany mxh Offenburg
In addition to that, can you agree that the following code is a bug as \Drupal\Core\Field\WidgetInterface::massageFormValues restricts $values to be an array while \Drupal\Component\Utility\NestedArray::getValue returns mixed types and nothing ensure values to be an array?
Im trying to explain, feel free to ask if you don't understand or need more clarification. I know this is a complex part.
I don't generally categorize places where a fatal may occur as a bug. Just because something throws a fatal error, does not always mean the place where it happens is the place where the but resides. There are valid cases where a fatal error to be thrown is intentional, for example when passing along invalid parameter values. It then enforces triggering / invoking parts to fix passing of invalid arguments or wrong times of invocation. That's basically my point of #17, that this is a runtime problem.
The code part you ask me about in #17 is extracted from
Drupal\Core\Field\WidgetBase::extractFormValues::extractFormValues
right? The difficulty here is that this is one of the most complex layers of Drupal, the form API, which is extremely dependent on the correct timings and order of invocations, and where it's extremely hard to see what the right thing is to do in what circumstance. I personally don't like it and find it too complex. I also still struggle with it when working with it. But it is what it is, and when making use of it, the triggering part needs to be fixed that is will do the right calls at the right time and in the right order. - 🇩🇪Germany jurgenhaas Gottmadingen
Thanks for explaining. I can follow your logic and agree with your conclusions. However, in the context of safe coding, the code block in core should still do something about that potential type inconsistency and handle that explicitly, rather than implicitly assuming the type would be correct. There is even an issue about it (see 🐛 WidgetBase::extractFormValues relies on non guaranteed return value Needs work ) and at least 2 other use cases came across the same problem. The patch there would actually avoid the situation we face here as well, but as you say, this doesn't mean it's the correct approach.
- Merge request !440Issue #3457773 by gge, jurgenhaas, mxh: An error occurred while trying to... → (Merged) created by mxh
- Status changed to Needs review
about 1 month ago 1:42pm 30 July 2024 - 🇩🇪Germany mxh Offenburg
The problem is that the submit instruction
$handler->entityFormSubmit($form, $form_state);
is being called so early that form validation has not yet happened. However form validation is the layer where submitted form values are being converted to be compatible for themassageFormValues
instruction later on. One example isDrupal\Core\Field\Plugin\Field\FieldWidget\OptionsWidgetBase::validateElement
where the last instruction "normalizes" the submitted values with$form_state->setValueForElement($element, $items);
Created an MR !440 that adds in a manual form validation call. I hope that it doesn't break anything else - it's hard to tell.
- 🇩🇪Germany jurgenhaas Gottmadingen
This is impressive, so glad you know your way around the form API @mxh.
The MR looks good regarding the code itself. Leaving the issue at NR, though. Hoping that @gge can actually test this in his context to see, if the messages disappear.
- 🇷🇴Romania gge
Hello,
I can confirm that in my context the message disappear. I'm gonna stress the inline entity form build event for a while and will report of there will be any problems. For now everything seems to work very good!
Thank you very much!
- Status changed to RTBC
about 1 month ago 8:37am 1 August 2024 - 🇩🇪Germany jurgenhaas Gottmadingen
In that case, this is RTBC for the current issue. @mxh how do you feel about merging this into 2.1 and back porting to 2.0? If follow-ups occur, we can still open new issues then.
-
jurgenhaas →
committed 375c1a2d on 2.1.x authored by
mxh →
Issue #3457773 by mxh, gge, jurgenhaas: An error occurred while trying...
-
jurgenhaas →
committed 375c1a2d on 2.1.x authored by
mxh →
-
jurgenhaas →
committed 94c22d08 on 2.0.x
Issue #3457773 by mxh, gge, jurgenhaas: An error occurred while trying...
-
jurgenhaas →
committed 94c22d08 on 2.0.x
- Status changed to Fixed
about 1 month ago 9:51am 1 August 2024 Automatically closed - issue fixed for 2 weeks with no activity.
- 🇩🇪Germany jurgenhaas Gottmadingen
ATTENTION: this fix causes trouble with the complex IEF widget, see details at 🐛 ECA Form breaks complex IEF widget Active .