Support from Acquia helps fund testing for Drupal Acquia logo

Comments

chr.fritsch created an issue. See original summary.

chr.fritsch’s picture

Status: Active » Needs review
Issue tags: +D8Media
FileSize
4.48 KB

Here is a patch

mtodor’s picture

FileSize
5.04 KB
574 bytes

Added missing schema.

naveenvalecha’s picture

Status: Needs review » Needs work
Issue tags: +Needs tests

Nice work @mtodor, Quick review to move this forward. Having tests will be great.

  1. +++ b/modules/entity_form/src/Plugin/EntityBrowser/FieldWidgetDisplay/InlineEntityForm.php
    @@ -0,0 +1,131 @@
    +    if ($entity->access('update', \Drupal::currentUser())) {
    

    It would be nice to inject the inject the current service.

  2. +++ b/modules/entity_form/src/Plugin/EntityBrowser/FieldWidgetDisplay/InlineEntityForm.php
    @@ -0,0 +1,131 @@
    +    var_dump($dependencies);
    

    One of my favourite function but we need to remove it.

mtodor’s picture

Assigned: Unassigned » mtodor
mtodor’s picture

Assigned: mtodor » Unassigned
Status: Needs work » Needs review
FileSize
8.79 KB
10.36 KB

Let's try to move this forward. All issues listed in #4 should be addressed with provided patch.

volkerk’s picture

Status: Needs review » Needs work

InlineEntityForm is returned as a entity display leading to paragraphs 'collapse' button triggering the wrong widget: (EntityReferenceBrowserWidget->extractFormValues()) which has no implementation for handling the state of the InlineEntityForm.
Also the structure of the form_state is messed up by this (additional 'display' key in parents).

Steps to reproduce:

1) Create node type with paragraph field.
2) Create paragraph type with entity reference.
3) Use EntityBrowser widget and configure its ENTITY DISPLAY PLUGIN to 'inline_entity_form'.
3) Create node, add paragraph, add referenced entity.
4) Save the node.
5) Edit node; edit paragraph; change values in paragraph form.
6) Collapse the paragraph form.
7a) Save the node > changes will be lost.
7b) Open paragraph form > changes will be lost.

LeStat’s picture

Version: 8.x-1.x-dev » 8.x-2.x-dev
LeStat’s picture

FileSize
5.4 KB

Patch update for 8.x-2.x.

LeStat’s picture

FileSize
5.66 KB
Vidushi Mehta’s picture

Status: Needs work » Needs review
LeStat’s picture

LeStat’s picture

FileSize
5.44 KB

The last submitted patch, 12: 2858438_12.patch, failed testing. View results

pminf’s picture

Could someone please provide a more detailed description or some screenshots what to expect? I assume, that after selecting one or multiple entities in the entity browser the corresponding forms of these selected entities should be open immediately. Using patch from #6 this is not the case and I also can't find an additional form display widget setting for this.

Primsi’s picture

I see #6 also included tests. Are those not usable in the latest patches?

chr.fritsch’s picture

Issue tags: -Needs tests
FileSize
10.42 KB

Bringing back the tests from #6 to #13

Status: Needs review » Needs work

The last submitted patch, 17: 2858438_17.patch, failed testing. View results

chr.fritsch’s picture

Status: Needs work » Needs review
FileSize
1.14 KB
10.4 KB

Removed ctools dependency and switched to WebDriverTestBase

oknate’s picture

Status: Needs review » Needs work
FileSize
567.77 KB

I tested this briefly. Overall it works nicely.

1) On a default install, when I hover over the form, it's greyed out (https://www.drupal.org/files/issues/2019-07-07/greyed-out.mov). This is with seven admin theme and an entity browser using modal display, single widget selector plugin, no selection display and one view widget.

.entities-list.sortable .item-container:hover {
    cursor: move;
    opacity: 0.6;
}

2) On a default entity browser field there's a separate edit button that opens a modal. I would think if you are using this display, that should be suppressed. I guess it's easy enough to turn off. So that's not a must-have. But maybe we could suppress it when using this display?
3) is there a way to have the inline entity form closed?

tstoeckler’s picture

Just found this patch, have something similar (although not quite as elegant ;-)) on a project as well. Still trying to figure out some issues, but here's one thing I noticed:

+++ b/modules/entity_form/src/Plugin/EntityBrowser/FieldWidgetDisplay/InlineEntityForm.php
@@ -0,0 +1,144 @@
+    $options = parent::settingsForm($form, $form_state);
+
+    foreach ($this->entityDisplayRepository->getFormModeOptions($this->configuration['entity_type']) as $id => $form_mode_label) {
+      $options[$id] = $form_mode_label;
+    }

This can just be $options = $this->entityDisplayRepository->getFormModeOptions($this->configuration['entity_type'])

The parent call is bogus, it just happens to work because the parent returns an empty array. And the foreach is unnecessary.

tstoeckler’s picture

Some more nitpicks:

  1. +++ b/config/schema/entity_browser.schema.yml
    @@ -201,6 +201,14 @@ entity_browser.field_widget_display.rendered_entity:
    +entity_browser.field_widget_display.inline_entity_form:
    

    I think this should be added to entity_browser_entity_form.schema.yml because the plugin is (rightly) added to entity_browser_entity_form not to entity_browser.

  2. +++ b/modules/entity_form/src/Plugin/EntityBrowser/FieldWidgetDisplay/InlineEntityForm.php
    @@ -0,0 +1,144 @@
    +class InlineEntityForm extends FieldWidgetDisplayBase implements ContainerFactoryPluginInterface {
    

    I think this should be called EntityForm for consistency with the EntityBrowser/Widget plugin.

  3. +++ b/modules/entity_form/tests/src/FunctionalJavascript/InlineEntityFormFieldWidgetDisplayTest.php
    @@ -0,0 +1,152 @@
    +  /**
    +   * Modules to enable.
    +   *
    +   * @var array
    +   */
    +  public static $modules = [
    

    Should be protected and use @inheritdoc

tstoeckler’s picture

Two more nitpicks, and then something substantial ;-):

  1. +++ b/modules/entity_form/src/Plugin/EntityBrowser/FieldWidgetDisplay/InlineEntityForm.php
    @@ -0,0 +1,144 @@
    +    foreach ($this->entityDisplayRepository->getFormModeOptions($this->configuration['entity_type']) as $id => $form_mode_label) {
    ...
    +    if ($form_mode = $this->entityTypeManager->getStorage('entity_form_mode')->load($this->configuration['entity_type'] . '.' . $this->configuration['form_mode'])) {
    

    this->configuration['entity_type'] is not declared anywhere. So I think we need to add that setting and add some Ajax logic to the form which dynamically updates the available form modes when you update the entity type.

  2. +++ b/modules/entity_form/tests/src/FunctionalJavascript/InlineEntityFormFieldWidgetDisplayTest.php
    @@ -0,0 +1,152 @@
    +class InlineEntityFormFieldWidgetDisplayTest extends WebDriverTestBase {
    

    Similar to the plugin itself, we can drop the "Inline" part of the class name here for consistency with the other test.

Now the interesting part:
The problem I was having with my custom implementation of this was that nesting another inline entity form inside of the inline entity form embedded here would lead to the inner form values not being submitted. I can reproduce this on a clean install with this patch. Will add a failing test for that and hopefully a fix, we'll see...

tstoeckler’s picture

Status: Needs work » Needs review
FileSize
7.95 KB
6.49 KB
15.54 KB

OK here's a (failing) test to prove my problem. Didn't fix the nitpicks mentioned above yet. I did go ahead and change the test a bit to use exported configuration as that seems to be more in-line with the rest of the Entity Browser test coverage. Thus, two interdiffs. One, just with the refactoring and a second one with the test coverage for the nested form. I amended that into the existing test instead of creating a separate test method, as - even though it's somewhat of a different use-case - testing the nested case requires properly filling in values for the "middle" entity, so we end up duplicating the "non-nested" test coverage. I'm certainly up for suggestions to improve the structure of the test, however.

Will not get around to a fix today, I guess, hopefully tomorrow.

Status: Needs review » Needs work

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

tstoeckler’s picture

Status: Needs work » Needs review
FileSize
3.51 KB
6.46 KB
16.94 KB

Alright, this makes the test pass. I will try if this also fixes the issue for my project. I had also looked at entity_browser_entity_form_inline_entity_form_reference_form_alter() and entity_browser_entity_form_reference_form_validate() and entity_browser_entity_form_reference_form_submit() which the former sets up, but I didn't have much lookluck with that yet. To be honest the intimate details of both Inline Entity Form as well as Entity Browser are a still very cloudy to me, so anything that works is good enough for me.

I took the time to fix up the in #21 / #22 / #23 and also some other small things I noticed. I left a @todo for the entity type configuration for now didn't want to get held up on that now.

Again providing two interdiffs: One for the test fix, one for the cleanups.

Edit: Minor language fail

tstoeckler’s picture

OK, this does seem to fix the problem for me, so I think we can go ahead here. Reviewed the code once more with a bit more detail, found a bunch of nitpicks, but nothing important... (well but for the missing entity type select, per the above).

  1. +++ b/modules/entity_form/src/Plugin/EntityBrowser/FieldWidgetDisplay/EntityForm.php
    @@ -0,0 +1,165 @@
    +use Drupal\Core\Session\AccountProxyInterface;
    ...
    +   * @var \Drupal\Core\Session\AccountProxyInterface
    ...
    +   * @param \Drupal\Core\Session\AccountProxyInterface $user
    ...
    +  public function __construct(array $configuration, $plugin_id, $plugin_definition, EntityTypeManagerInterface $entity_type_manager, EntityDisplayRepositoryInterface $entity_display_repository, AccountProxyInterface $user, ElementInfoManagerInterface $element_info_manager) {
    

    I think just AccountInterface should be fine here.

  2. +++ b/modules/entity_form/src/Plugin/EntityBrowser/FieldWidgetDisplay/EntityForm.php
    @@ -0,0 +1,165 @@
    +   * Entity manager service.
    ...
    +   *   Entity type manager service.
    

    "The entity type manager."

  3. +++ b/modules/entity_form/src/Plugin/EntityBrowser/FieldWidgetDisplay/EntityForm.php
    @@ -0,0 +1,165 @@
    +   * Entity display repository.
    ...
    +   *   Entity display repository service.
    

    "The entity display repository."

  4. +++ b/modules/entity_form/src/Plugin/EntityBrowser/FieldWidgetDisplay/EntityForm.php
    @@ -0,0 +1,165 @@
    +   * Current user.
    ...
    +   *   Current user.
    

    "The current user account."

  5. +++ b/modules/entity_form/src/Plugin/EntityBrowser/FieldWidgetDisplay/EntityForm.php
    @@ -0,0 +1,165 @@
    +      // widget submit handlers. Make sure, however, not to override the
    

    Oops, forgot to finish the sentence here.

  6. +++ b/modules/entity_form/src/Plugin/EntityBrowser/FieldWidgetDisplay/EntityForm.php
    @@ -0,0 +1,165 @@
    +      'entity_type' => \Drupal::moduleHandler()->moduleExists('node') ? 'node' : 'user',
    

    Inject the module handler

  7. +++ b/modules/entity_form/src/Plugin/EntityBrowser/FieldWidgetDisplay/EntityForm.php
    @@ -0,0 +1,165 @@
    +    if ($form_mode = $this->entityTypeManager->getStorage('entity_form_mode')->load($this->configuration['entity_type'] . '.' . $this->configuration['form_mode'])) {
    +      $dependencies[$form_mode->getConfigDependencyKey()][] = $form_mode->getConfigDependencyName();
    +    }
    

    There should not be an if here, the form mode should be assumed to exist.

  8. +++ b/modules/entity_form/tests/src/FunctionalJavascript/EntityFormFieldWidgetDisplayTest.php
    @@ -0,0 +1,147 @@
    + * Test for entity form field widget display.
    

    "Tests the ..."

  9. +++ b/modules/entity_form/tests/src/FunctionalJavascript/EntityFormFieldWidgetDisplayTest.php
    @@ -0,0 +1,147 @@
    + * @covers \Drupal\entity_browser_entity_form\Plugin\EntityBrowser\FieldWidgetDisplay\InlineEntityForm
    

    I think this is somewhat non-standard for functional tests and also not consistent with the other test, so I would vote to remove it.

  10. +++ b/modules/entity_form/tests/src/FunctionalJavascript/EntityFormFieldWidgetDisplayTest.php
    @@ -0,0 +1,147 @@
    + * @package Drupal\Tests\entity_browser_entity_form\FunctionalJavascript
    

    Let's remove this.

  11. +++ b/modules/entity_form/tests/src/FunctionalJavascript/EntityFormFieldWidgetDisplayTest.php
    @@ -0,0 +1,147 @@
    +  /**
    +   * User permissions for the logged-in user.
    +   *
    +   * @var string[]
    +   */
    +  protected static $userPermissions = [
    

    "The ..."

    Also not sure what the value is of putting this is a separate variable, in particular why making it static?

  12. +++ b/modules/entity_form/tests/src/FunctionalJavascript/EntityFormFieldWidgetDisplayTest.php
    @@ -0,0 +1,147 @@
    +    'bypass node access',
    

    It's a minor point, but I think we could be more explicit by removing this permission and adding the "actual" ones that are needed, i.e. "view article content", "update article content", etc.

volkerk’s picture

FileSize
17.12 KB
6.41 KB

I addressed all points from #27 except 11. and 12. for which I could not find a solution.

I would like to make a point against changing the plugin id from “inline_entity_form" to “entity_form”.

This patch is available for over 2 years now and lots of people are using it, the naming change will break their configuration. Providing an update hook would be a solution, but it only ever has been a patch.

Status: Needs review » Needs work

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

volkerk’s picture

FileSize
16.77 KB
5.16 KB
6.41 KB

Rollback 6. Inject module handler: defaultConfiguration() is called in constructor.

volkerk’s picture

Status: Needs work » Needs review
sokru’s picture

Not sure how to this should have been tested so I used following steps:

1. drush en content_translation, language, entity_browser, media_entity_browser_media_library, media_entity_browser_media, entity_browser_entity_form, inline_entity_form -y
2. Add a another language /admin/config/regional/language
3. Add new entity reference field to article content type. Use "Users may translate this field".
4. Use EntityBrowser widget and configure its ENTITY DISPLAY PLUGIN to 'Entity form'.
5. Create new article and add media entity.
6. Translate the article and edit media entity alt text.

I expected alt text of media entity to be translated but its not, instead the source language media entity alt text got changed.
Saving the node gives following php warnings:

Warning: krsort() expects parameter 1 to be array, null given in Drupal\inline_entity_form\WidgetSubmit::doSubmit() (line 47 of modules/contrib/inline_entity_form/src/WidgetSubmit.php).
Warning: Invalid argument supplied for foreach() in Drupal\inline_entity_form\WidgetSubmit::doSubmit() (line 48 of modules/contrib/inline_entity_form/src/WidgetSubmit.php).