Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
I assume the desired behavior is that only the top level parent has these and the other nodes act the same as the parent node. But what if the top level parent isn't a node?
Comment | File | Size | Author |
---|---|---|---|
#16 | interdiff.txt | 6.16 KB | tedbow |
#16 | ief-rewrite_the_base_inline-2667710-16.patch | 13.51 KB | tedbow |
| |||
#13 | ief-rewrite_the_base_inline-2667710-13.patch | 14.73 KB | tedbow |
| |||
#11 | Screenshot 2016-02-17 12.55.34.png | 136.6 KB | tedbow |
#10 | ief-rewrite_base_inline_form-2667710-10.patch | 3.18 KB | tedbow |
|
Comments
Comment #2
tedbowComment #3
tedbowComment #4
tedbowDrupal 7 screenshot
Each nested node has "Published checkbox but no other properties.
Comment #5
tedbowOk here is patch that removes all element in node forms that will show up under "advanced" section.
Should note that this already the behaviour unless the field field is required and the form is created when the parent form is created and not in ajax callback.
Comment #6
tedbowAdding tests to patch and TEST_ONLY patch
Comment #8
tedbowSetting to "Need Review" because only TEST_ONLY patch failed as expected.
Comment #9
bojanz CreditAttribution: bojanz at Centarro commentedThe problem is much bigger than that.
This code gets a form object for the given entity type, then passes it the form.
In the node case, $controller is an instance of Drupal\node\NodeForm.
However, entity forms are not designed to be embedded, they assume they own the form.
Hence, endless conflicts.
We need to rewrite the EntityInlineForm logic completely, and use the form display object directly, just like ContentEntityForm does:
Plus the relevant logic in buildEntity / validateForm, etc.
Basically, the InlineForm is an alternative to a ContentEntityForm, and needs to have the same logic (equivalent of D7's field_attach_form).
Comment #10
tedbowOk, as a start I just replaced:
$controller = $this->entityManager->getFormObject($entity_form['#entity']->getEntityTypeId(), $operation, FALSE);
with
I also removed the test that was checking that advanced properties don't show up.
So now the advanced properties will show up if have the permissions.
Comment #11
tedbowOk, the current tests pass but all the test are done with "administer nodes" permission so it might not see all the form conflicts
I did check manually for now "advanced" tabs base fields are not duplicated
Should we have test that uses xpath to check that base fields like "authored by" and "authored on" are in the correct part of the form.
Right now it doesn't act like Drupal 7 version because these properties don't show up for child forms.
With #2510274: Add ability to select form Display Mode a sitebuilder could use a different form display to not show authored by, authored on, promoted, and sticky. but this would a pain if you had a number of content types. Also the revision log can't be hidden by form display screen.
I also did a quick test that I could have different author and publication date on a child node. Would this be a good test?
Comment #12
bojanz CreditAttribution: bojanz at Centarro commentedtedbow and myself did some exploration of the problem, here's an update.
When it was time to design IEF for D7, I first looked at subform, which tried to simulate an entire form lifecycle (building, validation, submission), leaving the underlying form oblivious of the embedding. The subform code was long and hard to understand, so instead I decided to imitate field widgets, creating entity forms that are aware of the fact that they are being embedded (via $entity_form['#parents']). Just like field widgets, it meant that their validation and submission methods had to rely on #parents to get the right values from form state. Of course, this meant that the usual form hooks didn't fire, IEF added its own instead.
The assumption was that the IEF context is sufficiently different to warrant different forms (there's simply less screen space), which made the idea of embedding an existing form unattractive anyway.
This is how we arrived at the current D7 situation.
D8 made an accidental mistake, and started calling the original entity forms (ContentEntityForm, represented as $controller) instead. These forms always assume that they own the entire form, not respecting #parents at all, meaning that all kinds of problems are possible (since multiple forms are fighting over the same part of $form_state).
Instead, we go back to the D7 way. The D7 field_attach_* methods have migrated to the EntityFormDisplay object.
That means our code needs to get a form display:
and then use the $form_display object to call its buildForm() / validateFormValues() and other methods.
tedbow raised the concern that this would cause us to duplicate all of ContentEntityForm and EntityForm, so I looked into the extra logic those two classes have.
All of ContentEntityForm logic revolves around allowing forms to add form elements for entity fields, instead of using widgets. I am guessing this is due to legacy reasons. That's not something we need to support in IEF at this point, we should be pushing people to use widgets as much as possible. Almost no entity type should need to override the entityForm methods of the InlineForm handler.
The only use case for not using a widget, but doing manual form elements instead is having to cover multiple fields at the same time, but that's an edge case that I'm happy to address in a different issue (one that perhaps has less code than ContentEntityForm).
When it comes to EntityForm, a part revolves around getting the entity from the route (unneeded) and firing various hooks (unneeded). Another part revolves around building $this->entity (as an alternative to us using $entity_form['#entity'] all over the place), but we can do that as cleanup in a different issue too.
Comment #13
tedbowOk here is the first try at it.
I removed all need to get the $controller as in
$controller = $this->entityTypeManager->getFormObject($entity_form['#entity']->getEntityTypeId(), $operation, FALSE);
This removes the need for $child_form_state.
All test pass for me locally. I did have to change 1 line in InlineEntityFormElementWebTest but i wasn't sure when the original was passing before and the newer 1 is more specific.
Comment #14
bojanz CreditAttribution: bojanz at Centarro commentedInvalid docblock, extra newline after opening brace.
There's no reason for this code to live in a separate method, put it back in entityForm().
Also remember that @var typehints need the full namespace.
Do we still need this?
Invalid docblock. Verb must be Gets, interface must have namespace, param and return must have descriptions.
We didn't port any other logic that allows covering entity fields not using widgets, so the only piece of this method that works is
$extracted = static::getFormDisplay($entity, 'default')->extractFormValues($entity, $form, $form_state);
We might as well remove the entire method and move that line to the calling method.
Comment #15
tedbowIt is still true that entityFormValidate will validate the entity.
It calls validateFormValues which says:
But I am going to move it here.
Because I think the only reason is to avoid double validation on save and if we aren't saving it seems useless.
Comment #16
tedbowChanges asked for in #14 excepted 1 noted in #15
Comment #18
bojanz CreditAttribution: bojanz at Centarro commentedTweaked the docs and committed. 140 lines of code less, and who knows how many bugs. Thanks, tedbow.
Comment #19
slashrsm CreditAttribution: slashrsm at Examiner.com commentedGreat progress. Thank you both! I am sorry I wasn't able to be involved.