Dear IEF developers and users,

I am wondering, why the single value inline entity form widget does not support the selection of existing entities?

In my project requirement, the user should be able to choose if he want to use an existing entity or create a new one.

I -could- use the multi value field and limit it to only 1 value, but this is ugly in user experience perspective:

  • "You have added 0 out of 1 allowed" is a useless information
  • The draggable list of values is also useless, we only have one possibility
  • The user has to click "edit" when he wants to view and edit the fieldvalues of the entity

It should be not that hard to alter this via hook_form_alter(), but before I start the hackathon, give me some please some informations about that. If you think this could be a good improvement for the module, I am inclined to create a patch for this!

Greetings,
axe312

CommentFileSizeAuthor
#99 inline_entity_form-switch_entity.patch8.06 KBnetw3rker
#93 inline_entity_form-allow-to-add-existing-2134035-93.patch43.77 KBnetw3rker
#91 inline.png16.28 KBsassafrass
#90 inline_entity_form-allow-to-add-existing-7.x-1.8-2134035-89.patch45.44 KBraines37
#82 inline_entity_form-Allow_to_add_existing-2134035-82-7.x.patch67.54 KBdunebl
#76 inline_entity_form-allow-to-add-existing-2134035-76.patch44.04 KBdas-peter
#65 inline_entity_form-allow-to-add-existing-2134035-63.patch42.03 KBdas-peter
#63 inline_entity_form-allow-to-add-existing-2134035-63.patch42.63 KBdas-peter
#63 interdiff-2134035-59-63-do-not-test.diff3.12 KBdas-peter
#59 inline_entity_form-allow-to-add-existing-2134035-59.patch41.97 KBdas-peter
#52 inline_entity_form-allow-to-add-existing-2134035-52-ignore-whitespace.patch33.76 KBpfrenssen
#52 inline_entity_form-allow-to-add-existing-2134035-52.patch41.45 KBpfrenssen
#48 inline_entity_form-allow-to-add-existing-2134035-48.patch41.59 KBrealityloop
#39 interdiff.txt4.88 KBpfrenssen
#37 inline_entity_form-allow-to-add-existing-2134035-36.patch41.59 KBdas-peter
#37 interdiff-2134035-30-36-do-not-test.diff13.65 KBdas-peter
#30 allow_to_add_existing-2134035-30-ignore-whitespaces.patch29.42 KBaxe312
#30 allow_to_add_existing-2134035-30.patch49.53 KBaxe312
#28 2134035-28-inline_entity_form-ignore_whitespace.patch29.42 KBpfrenssen
#28 2134035-28-inline_entity_form-add_existing_single_value.patch49.53 KBpfrenssen
#24 2134035-24-inline_entity_form-ignore_whitespace.patch3.97 KBpfrenssen
#24 2134035-24-inline_entity_form-add_existing_single_value.patch24.08 KBpfrenssen
#22 entity_selection_handler.png190.08 KBarpitr
#12 ief_single_existing-2134035-12.patch45.81 KBbendiy
#11 ief_single_existing-2134035-11.patch45.62 KBbendiy
#10 ief_single_existing-2134035-10.patch44.18 KBbendiy
#9 ief_single_existing-2134035-09.patch44.76 KBbendiy
#8 ief_single_existing-2134035-08.patch43.53 KBbendiy
#5 multi-add-at-once-existing.png13.81 KBbendiy
#5 add-existing-widget-selection.png14.42 KBbendiy
#5 single-add-existing.png9.38 KBbendiy

Comments

bendiy’s picture

I'd love to see this feature as well. My use case requires end users to be able to choose an existing entity or add a new one. I'm finding the multi-value widget way too confusing for this. I'm going to have to find a solution to this soon.

What I'd like to see/make is a Single Value widget that is similar to how the AddressField and AddressBook modules work in Drupal Commerce. You would have a select list or auto complete at the top and then form below it. If you select an existing entity, the form auto fills with those values. Or you can use the form to enter a new entity.

axe312’s picture

@bendiy: I also need this functionality soon. We may invest some time together to create a patch?

Can one of the IEF devs pls help us in developing the patch or provide some feedback how we can achieve this functionality in a good way?

bendiy’s picture

@axe312 I'll probably start on it after next week. Keep me updated if you make any progress. I'll do the same.

bendiy’s picture

@axe312 I've almost got this working. Have you made any progress on this feature? I should have a patch next week.

bendiy’s picture

This is what it looks like:
single-add-existing

axe312’s picture

Hi bendiy,

i had no time to do any work on this feature at the moment. Can you post your code as a patch? I will review and improve it :)

Greetings

bendiy’s picture

@axe312
See the attached patch. I've got it working for the single widget. You will need to set the widget settings to support this:
add-existing-widget-selection

@bojanz
This patch rolls up all the related issues. I know it's a lot of changes and hard to review. It will take me a while to roll this back and break it up into smaller patches. The new features are:

  1. Remove the IEF Autocomplete widget and replace it with widget settings options to use the existing EntityReference (Select List, Radio buttons, Autocomplete, Autocomplete(Tag Style)). It's not using the plugin system, but it does work with the existing widgets. #1807120: Make the "Add existing variation" widget pluggable
  2. Add support to add multiple entities at once if cardinality > 1 using (Select List, Radio buttons, Autocomplete(Tag Style)). Just select mutiples and click the "Add {entity}" button.
  3. Add support for existing entity selection to the single widget. You have to first set the widget to "Allow Existing". Then you can pick which EntityReference widget to use. That widget will be added above the entity form. When an entity is selected, the form will auto-fill with that entity's info and the relation will be set. You can change this by choosing a different entity. If you don't select any entity, filling out the form will create a new entity as it always has. #8258065 this issue
  4. Add a hook_inline_entity_form_default_entity_alter() to allow other modules to set a default entity before the IEF is loaded.
  5. Add form element tags to all buttons and ajax elements ('#ief_processing' => true and '#ief_id') to properly fix the inline_entity_form_process_entity_form() bugs. This removes all of the '#ief_submit_all' tagging which doesn't work on multi-step forms like Drupal Commerce's checkout panes and assumed what the submit button was. Instead, this fix tags all the IEF actions and looks for those actions when processing the form submits. #2032649: Rework inline form submission
  6. Fix: #2120817: inline_entity_form_process_entity_form() creates duplicate entities when validation error requires "Save" be clicked twice.
  7. Fix: #2118841: Autocomplete is too agressive in stripping the results list
  8. Fix: #1891652: "Required" form setting ignored with multiple values widget
  9. Fix: #1839676: Add hook_inline_entity_form_settings_alter()
bendiy’s picture

Component: User interface » Code
StatusFileSize
new44.76 KB

Here's a new patch. It has all of the above, with a few changes.

  1. Missed a few code cleanup tasks.
  2. Fix handling when $field['cardinality'] == FIELD_CARDINALITY_UNLIMITED
  3. Fix a bug when using field_collections from comment #18 at #2032649: Rework inline form submission
bendiy’s picture

StatusFileSize
new44.18 KB

Oops, my clean up for reversing this was wrong. #2117637: Seemingly incompatible with other content types

The attached patch should be correct, but you will need to apply the patch at the Entity API issue #1780646: entity_access() fails to check node type specific create access.

bendiy’s picture

StatusFileSize
new45.62 KB

This patch fixes relations using the Product Reference field type. It make sure to use the correct column name and preserves the autocomplete_path.

bendiy’s picture

StatusFileSize
new45.81 KB

This fixes an issue with field_collections. It get's the field_collection_item entity's item_id and uses that as the child delta in $ief_id to preserve the right field_collections mapping to the IEF.

milos.kroulik’s picture

Latest patch doesn't apply to IEF 1.3. Error was:

error: patch failed: inline_entity_form.module:373
error: inline_entity_form.module: patch does not apply
bendiy’s picture

@milos.kroulik

You need to be on the latest dev version, not 1.3.

milos.kroulik’s picture

Thanks, that worked correctly. Interesting is, that I used yor patch to solve entirely different issue (I am using Address field in referenced node and it was not saving properly).

bojanz’s picture

Status: Needs review » Needs work

I cannot review or commit this patch because it has fixes for 10 issues at once.
One patch per bug / feature request. That's how it goes.

The 7/8/9 issues you mention in #8 now have final fixes committed.
There is no need for this issue to care about nested IEFs or reworking IEF submission.
We already have another issue for that.

bendiy’s picture

@bojanz Understood. Like I said above, there's a lot changing here. My main goal was to get it into other people's hands for testing.

Thanks for committing fixes for the related issues, that will make it much easier to split this up into the 3 or 4 issues that remain and re-roll patches for them. I'll try and find some time for that soon.

axe312’s picture

Very nice to come back from the holidays and seeing a lot of progress here.

Can you tell me, where to look at and whats ready to test?

bendiy’s picture

@axe312 The patch in #12 should work for single value relations. It also includes fixes for everything in #8. bojanz has committed fixes for several of those bugs since #12 was created. #12 will probably need to be rerolled or you will need to apply it to an eariler version of dev before bojanz committed those fixes.

The patch at #12 needs to be split up into 3 or 4 patches to address those other open issues.
#1807120: Make the "Add existing variation" widget pluggable
#8258065 this issue
#2032649: Rework inline form submission my fix for this is different from what bojanz committed. Need to figure out what changes from #12 should be combined with bojanz fix after some testing.
There are a few other minor changes listed in #8 that might need to be broken out.

I've moved on to some other pressing tasks. I probably won't be able to circle back and finish this until next month.

arpitr’s picture

I tried using patch #12 after re rolling the module to

commit 99ca20df233b76be3b37625366663e5bc8ba32e8
Author: Bojan Zivanovic
Date: Thu Dec 6 01:28:17 2012 +0100

Issue #1808398: Create a 'Single' widget instead of activating it implicitly.

but patch is not applying.

bendiy’s picture

@arpitr You can try my full fork over here with all changes applied:
https://github.com/CodeDrivenDrupal/inline_entity_form/tree/xtuple

I'm still committed to merging those changes back into IEF, but I don't know when I'll have time for that.

arpitr’s picture

StatusFileSize
new190.08 KB

@bendiy thanks for your reponse. I tried your copy of module with entity reference with using "Inline entity form - Single value" widget but while configuring the reference field I got entity selection handler broken. Adding screenshot to show the problem
Entity Selection Handler

bendiy’s picture

@arpitr You can use the entityreference_efq_views module to give you selection handlers. That's what I've been doing. There are also be some bugs in entityreference that I've fixed in my fork: https://github.com/CodeDrivenDrupal/entityreference/tree/develop.

pfrenssen’s picture

I tried to clean up the patch from #12 but I couldn't make heads or tails of it. So I made a new one from scratch.

I did some manual testing on this with all combinations of settings and this initial version seems to work quite well already. I didn't do any testing in context of the Commerce platform. I will do more extensive testing over the coming weeks while I integrate this in my project.

I also want to write a test for this, but as there are no pre-existing tests it is quite hard for me to make assumptions about what the expected behaviour of the module actually is, especially seeing this is primarily intended for the Commerce platform which I am not well versed in. If this is OK for the maintainers I'll write a basic test that checks this issue while configuring it through the user interface.

There is a difference in the markup now for the original single value forms. Originally the outer container was a fieldset, and the inner container a div, and now it is the other way around. This has no impact on the base functionality but existing implementations might want to verify everything still looks as expected, especially if they are altering the form.

The patch is quite large because it removed a lengthy if-else statement but these are mainly whitespace differences. I also uploaded a diff that ignores the whitespace which is a bit easier to review :)

pfrenssen’s picture

Title: Why does "Inline entity form - Single value"-widget not support adding a existing entity » Allow to add existing entities using the single value field widget
Version: 7.x-1.3 » 7.x-1.x-dev
Category: Support request » Feature request
Status: Needs work » Needs review
deciphered’s picture

Status: Needs review » Needs work

@pfrenssen,

Unfortunately the cleanup/rewrite of the patch loses one of the key features of the patch, the ability to chose a Widget type for the existing reference field. While #19 recommends the splitting of this feature into another issue, which is the correct recommendation, removing it without separating the parts does mean it could easily be forgotten.

pfrenssen’s picture

Issue tags: +Needs tests

Good point! I created #2290863: Unify the widgets for single and multiple values as a followup to address that issue, and referred to the patch from #12.

Leaving this to "needs work" since it still needs tests.

pfrenssen’s picture

Status: Needs work » Needs review
StatusFileSize
new49.53 KB
new29.42 KB

Added a test. I could not properly test this issue on its own without covering the entire functionality of the single value field widget, so it ended up being a bit more work than expected.

I uploaded two versions of the patch, one of which is ignoring whitespace changes to make it easier to review. I did not bother with an interdiff since the patch itself did not change, just the test was added.

Automated testing is not yet enabled for this module so the tests won't run.

das-peter’s picture

Looks pretty good to me. And it works like a charm so far. However, I didn't test all scenarios yet.

axe312’s picture

Nice, thank you all so much for your hard work!

I made some tests and the patch is working very well. Here is a version of the patch which applies to the current git version.

fcortez’s picture

Where do you get the current git version?

das-peter’s picture

@fcortez Version control tab on the project page.

fcortez’s picture

@das-peter Of course I was looking for that in all the wrong places. LOL Thanks

pfrenssen’s picture

Issue tags: -Needs tests
das-peter’s picture

Status: Needs review » Needs work

I just figured out that this patch breaks the existing code in Commerce Discount (2.x).
Reason is that:

  1. Commerce Discount doesn't configure the create_bundles setting, thus no "Add entity" button is displayed. Previousely there was this code to handle such cases:
      if ($widget['type'] == 'inline_entity_form_single') {
        // Intentionally not using $settings['create_bundles'] here because this
        // widget doesn't care about permissions because of its use case.
        $bundle = reset($settings['bundles']);
  2. With this configuration inline_entity_form_entity_form() isn't called - and thus CommerceDiscountOfferInlineEntityFormController::entityForm() isn't invoked either.

Intentionally not using $settings['create_bundles'] here because this widget doesn't care about permissions because of its use case.

This suggests that we removed an use case with the patch.
I'm not to familiar with the code yet and just about to start patching. Feedback on how to toggle use cases would be welcome ;)

pfrenssen’s picture

Instead of checking if the widget type is "inline_entity_form_single", just check if the cardinality equals "1". That is what determines whether a field is a single value field or a multi value field.

das-peter’s picture

Status: Needs work » Needs review
StatusFileSize
new13.65 KB
new41.59 KB

Here we go.
The use case "single required entity" works now as before, which basically means it looks good and works again in Commerce Discount.
Let's see what the tests say.
Attention: The interdiff contains quite some "whitespace" / indentation changes.

joelpittet’s picture

Regarding #35 I was applying this patch in #37 and I noticed that it conflicted with another patch, so I wanted to crosslink them as they may be related to "create_bundles"
#2056139: Add option in setting to "Prevent adding a new entity".

pfrenssen’s picture

StatusFileSize
new4.88 KB

Here's the interdiff between #28 and #37 without whitespace.

Tip: you can add the "-w" option to git diff to ignore all whitespace changes:

$ git diff -w
pfrenssen’s picture

  1. +++ b/inline_entity_form.module
    @@ -616,12 +624,16 @@ function inline_entity_form_field_widget_form(&$form, &$form_state, $field, $ins
    +    if ($single_required_entity) {
    +      $bundle = reset($settings['bundles']);
    +      if (!empty($settings['create_bundles'])) {
             $bundle = reset($settings['create_bundles']);
    +      }
     
    

    I don't understand this part, will step through it.

  2. +++ b/inline_entity_form.module
    @@ -634,8 +646,14 @@ function inline_entity_form_field_widget_form(&$form, &$form_state, $field, $ins
    +  // If this is a single required entity with an exsiting entity open edit form.
    

    "Existing" ;)

pfrenssen’s picture

Status: Needs review » Needs work

Putting this to "Needs work" because we will need additional test coverage for the "single required entity" use case.

electrocret’s picture

Here's my use case for this feature. I am building a site for a local animal rescue group. On the adoption application they require vet contact info. Instead of having each applicant enter vet contact info (and making the database excessively larger with duplicate info), I was hoping on building a vet entity database which applicants can search and reference the vet entity on their application, and if the vet isn't in the database they can create it in their application for future applicants to use.

Entities:
Application -> Multi-inline entity form to Vet_Ref entities
Vet_Ref -> Entity contains application specific vet info, and a Single required entity to Vet_Info entity
Vet_Info -> Contains Generic Vet contact info.

Does anyone have a better way of implementing this?

socialnicheguru’s picture

@electrocret I am looking at something similar for locations.
My first instinct was to use inline_entity_form but the UI needed is not quite there yet.
I had to use reference_dialogue. It is a popup, but manages everything perfectly.

electrocret’s picture

@SocialNicheGuru My solution was to create an auto-complete entity reference field for existing vets, a check box boolean field for vet not found, and a single Inline Entity form for new vets. I used conditional fields to swap between the entity references if the vet not found check box was checked.

joelpittet’s picture

I tracked down a bug that relates to this issue: https://www.drupal.org/node/2432935#comment-9658407

Seems having this patch applied causes the commerce_discount offer field to not show up:(

pfrenssen’s picture

@joelpittet, I'm not very familiar with Drupal Commerce / Commerce Discount, can you give me some tips how I can quickly replicate the issue?

joelpittet’s picture

I think the easiest way is just grab the kickstart distribution because everything is setup. Then go to the add discount page. admin/commerce/store/discounts/add

The "Choose offer type" field will disappear with the patch applied.

The way that I was testing yesterday was just a stock drupal site, drush en -y commerce_discount
And going to the same page after the dependencies are enabled.
https://www.drupal.org/project/commerce_kickstart

You may be able to use simpletest.me for this too.

realityloop’s picture

reroll of 37

joelpittet’s picture

Thanks for the reroll @realityloop, My issue may be just some wierd conflict with another patch. I grabbed dev, applied that patch and cleared the registry and it didn't blow up...

Other patches I have applied: #1876652: Weight delta only supports ordering of 50 items, #2056139: Add option in setting to "Prevent adding a new entity"., #2187103: inline_entity_form_form_alter() incorrectly combines form level and button level submit handlers

1876652-ief-weight-delta-6.patch
2056139-prevent-add-new-2.patch
2187103.ief_form_alter.patch
joelpittet’s picture

Status: Needs work » Needs review

So I should set the status back.

socialnicheguru’s picture

no longer applies to latest dev or git pull.

pfrenssen’s picture

joelpittet’s picture

@pfrenssen awesome, I tried that re-roll and had to give up because of the conflicts and not knowing the changes needed. But inline_entity_form-allow-to-add-existing-2134035-52.patch applied perfectly and I had the older patch applied in my git repo and after upgrading the module and applying that patch there was no diff except the version number... so perfect reroll!

I'd like to RTBC but I really don't know the changes well enough to review the code. But I've been using this patch with no real problems to speak of. So if someone can do a code review on this to make sure things are as they should be?

RTBC+1

pfrenssen’s picture

In #2491527: [Drupal8] IEF FieldWidget merge & refactor an approach is discussed for merging the two widgets into one single widgets for the Drupal 8 port. This is very closely related to the patch in this issue which already unifies the rendering of the widget.

pfrenssen’s picture

das-peter’s picture

@realityloop & @pfrenssen Thanks for the re-rolls!!

bojanz’s picture

I have opened a wider discussion in #2532810: Decide on final widget UX.

das-peter’s picture

@bojanz Thanks!

Working on the next re-roll :)

das-peter’s picture

Here's a re-roll for the latest dev.

Status: Needs review » Needs work

The last submitted patch, 59: inline_entity_form-allow-to-add-existing-2134035-59.patch, failed testing.

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 59: inline_entity_form-allow-to-add-existing-2134035-59.patch, failed testing.

das-peter’s picture

Status: Needs work » Needs review
StatusFileSize
new3.12 KB
new42.63 KB

Hmm, the issues I've locally are:

  • The constructed node/add/* testing urls seem to contain underscores what's not appreciated.
  • InlineEntityFormSingleValueWidgetTestCase::assertInlineNode() seems to fail once in a while, but I couldn't figure out what's the trigger.
  • I see notices like DOMDocument::importNode(): ID inline-entity-form-3405694589f00056b40ed3fa3eeddd7f4933d323 already defined Warning drupal_web_test_case.php 2298 DrupalWebTestCase->drupalPostAJAX(): Not sure if this leads to a test fail but is probably problematic (?)

Changed the patch to ensure to use dashes instead underscore in node/add/* paths for testing.

Status: Needs review » Needs work

The last submitted patch, 63: inline_entity_form-allow-to-add-existing-2134035-63.patch, failed testing.

das-peter’s picture

Status: Needs work » Needs review
StatusFileSize
new42.03 KB

Again, without \r this time, hopefully ;)

Status: Needs review » Needs work

The last submitted patch, 65: inline_entity_form-allow-to-add-existing-2134035-63.patch, failed testing.

pfrenssen’s picture

Thanks a lot for working on this!

I see notices like DOMDocument::importNode(): ID inline-entity-form-3405694589f00056b40ed3fa3eeddd7f4933d323 already defined Warning drupal_web_test_case.php 2298 DrupalWebTestCase->drupalPostAJAX(): Not sure if this leads to a test fail but is probably problematic (?)

These are harmless. This is due to an update of the libxml2 library that is bundled in recent versions of PHP. The same issue was present in core: #2386903: Warning: DOMDocument::importNode() ID already defined. The libxml2 library now throws these warnings when a duplicate ID is encountered. This is something that can happen when you want to replace a part of the DOM with another piece of code that contains the same ID, since this requires the replacing code to be added to the DOM first before the original code can be removed.

It is very very hard to work around this since DOMDocument is not very flexible. I would suggest just to suppress these warnings by prefixing with an '@' symbol, just like was done in the core issue.

das-peter’s picture

@pfrenssen Thanks for letting me know about the libxml2 issue. I've update core locally to latest dev (was testing with an older version). Notices are indeed gone. However, the bad thing is that I don't have test fails locally and now not even a hint what could be wrong (131 passes, 0 fails, 0 exceptions, and 34 debug messages) :|

Ideas very welcome :)

pfrenssen’s picture

The tests are failing because the testbot doesn't realize the Entity Reference module is needed to run the tests. The test log shows that only the Entity and Inline Entity Form modules are being enabled, but not Entity Reference.

This patch responsibly adds a test dependency on the Entity Reference module to the .info file, but alas this is not picked up by the test bot until an actual release is made for Inline Entity Form that includes this line in the .info file. So these tests will only run as intended on the test bot when this patch is included and a new 7.x-1.7 release is made.

This sad state of affairs is a limitation of how the test bot works, and according to the relevant issue this cannot be fixed unfortunately: #1670936: Testbot does not detect/download additional dependencies.

das-peter’s picture

@pfrenssen Thanks! That reeeeally helps as I've encountered similar issues before and always wondered what the heck was going wrong.

milos.kroulik’s picture

The patch from #65 fails with latest dev version with:

error: patch failed: inline_entity_form.info:10
error: inline_entity_form.info: patch does not apply

Is there any other patch I should try?

pfrenssen’s picture

@milos.kroulik I just checked and the patch applies fine:

$ curl -s https://www.drupal.org/files/issues/inline_entity_form-allow-to-add-existing-2134035-63_0.patch | git apply

Are you perhaps downloading the dev release tarball from the project page? That has some metadata injected in the .info file by the packager. You can either remove the release metadata, or download the dev version with git.

milos.kroulik’s picture

@pfrenssen Given the file it fails on, that seems to be the case. Yes, I used development version downloaded by drush. Sorry for the noise.

pfrenssen’s picture

@milos.kroulik, no problem, I'm glad you solved it!

milos.kroulik’s picture

The patch currently fails with:

error: patch failed: inline_entity_form.module:477
error: inline_entity_form.module: patch does not apply

I tried to reroll the patch manually, but it doesn't seem to be straightforward and I don't want to introduce any issues. I can post hash of last commit usable with this patch, if it would help.

das-peter’s picture

Status: Needs work » Needs review
StatusFileSize
new44.04 KB

Attempted a re-roll, looks pretty good at least I couldn't find issues when testing manually and the tests pass too.
While testing it I stumbled over a (object comparison) recursion issue in inline_entity_form_reference_form_validate, if an entity contains a property referencing itself (e.g. revision handling). To fix this I changed the comparison from
if ($value['entity'] == $attach_entity) {
to
if (entity_extract_ids($entity_type, $value['entity']) == entity_extract_ids($entity_type, $attach_entity)) {
This should avoid (object comparison) recursions and work pretty well. I also had to adjust the flow a bit because the used unset($attach_entity); - now once the error is hit we leave the loop.

Status: Needs review » Needs work

The last submitted patch, 76: inline_entity_form-allow-to-add-existing-2134035-76.patch, failed testing.

milos.kroulik’s picture

The patch from #76 seems to be working fine for me. There should be perhaps more testers before RTBC.

sassafrass’s picture

I applied the latest patch successfully doing the following:

// Get the latest branch
git clone --branch 7.x-1.x http://git.drupal.org/project/inline_entity_form.git

// Get the latest patch
cd inline_entity_form
curl -s -O https://www.drupal.org/files/issues/inline_entity_form-allow-to-add-exis...

// Apply patch
patch -p1 < inline_entity_form-allow-to-add-existing-2134035-76.patch

// Result messages
patching file includes/entity.inline_entity_form.inc
patching file inline_entity_form.info
patching file inline_entity_form.module
patching file tests/single_value_widget.test

Success! Thank-you!

froboy’s picture

Think there's a line number difference in the .info file, but otherwise this patch works great and does exactly what it should.

ashzade’s picture

I followed #79 and got a few errors with the .info and .module. It's still somewhat working though.

dunebl’s picture

I have re-rolled #79 for the last version.
Let me know if this is ok.
If you have a field the IEF single value widget already setup:
1-Change to another widget
2-Change back to IEF single value
3-Click on "Modify" to change the field properties
4-Check: "Allow users to add existing"

pfrenssen’s picture

The last patch is not correct. It somehow grew 50% in size and the full test coverage from the previous patch is missing from it.

dunebl’s picture

You are right, but the changes are mostly coming from differences in the formatting... I used Eclipse IDE with some rules that change the formating... I am really sorry for that.

For example, a lot of changes are just the deletion of the comma of the last array item:

-    'controller' => 'NodeInlineEntityFormController',
+    'controller' => 'NodeInlineEntityFormController'

Anyway, I confirm that this patch is doing the job

joelpittet’s picture

@DuneBL could you set your editor to drupals coding standards for this patch so we don't need to weed through the changes in review? Since it's a reroll maybe even turn off your editors rewriting would suffice.

msharkeyiii’s picture

I have 3 "Inline entity form - Single value" fields in one EntityForm, immediately followed by an "Inline entity form - Multiple values" field. After applying the patch in #82, I'm getting this error:

Warning: asort() expects parameter 1 to be array, null given in inline_entity_form_field_widget_form() (line 729 of /****/modules/contrib/inline_entity_form/inline_entity_form.module).

Fixed by adding this code around line 729 in inline_entity_form.module (post-patch):

if (isset($bundles) && is_array($bundles)) {
  asort($bundles);
}
sassafrass’s picture

I tried to apply the latest patch to the latest version of 7.x-1.x-dev but got the following error/message:

Reversed (or previously applied) patch detected! Assume -R?

I assume it's because this latest patch was created against an older version of dev and needs to be re-rolled?

Thanks!

pfrenssen’s picture

The patch from #82 is not valid.

raines37’s picture

I'm attaching a reroll of #76 that should apply cleanly against 7.x-1.8.

inline_entity_form-allow-to-add-existing-7.x-1.8-2134035-89.patch

"works for me" ;)

raines37’s picture

sassafrass’s picture

StatusFileSize
new16.28 KB

Sorry for all the edits...

I applied the latest patch against 7.x-1.8. I didn't get any error messages; but it doesn't seem to have applied correctly.

I did a fresh Drupal install with the following modules: CTools, ECK, Entity API, Entity Reference, and Inline Entity Form. I created an Entity using ECK and added an Entity Reference field trying both single and multiple Inline widgets. Results:

The option to select Existing Entities doesn't display when using Inline Entity Form - Single Value (see attachment). Correct me if I'm wrong, but this isn't the behavior I was expecting. When using Inline Entity Form - Multiple Value, the Existing Entities option does display, but I do not see any difference in behavior. Essentially...this isn't working for me.

I opened the patch file to see if I could apply the patch manually. When I've done this in the past, the change lines have a single minus/plus sign, either a - or a +, and it was clear which lines were to be removed and which ones were to be added. But this patch file has multiple signs against one line of code. For example, ++ or +-. What does that mean?

Sample code excerpt:

+-    $hide_cancel = FALSE;
+-    // If the field is required and empty try to open one of the forms.
+-    if (empty($form_state['inline_entity_form'][$ief_id]['entities']) && $instance['required']) {
+-      if ($controller->getSetting('allow_existing') && !$controller->getSetting('allow_new')) {
+-        $form_state['inline_entity_form'][$ief_id]['form'] = 'ief_add_existing';
++  $hide_cancel = FALSE;
++  // Try to open the add form (if it's the only allowed action, the
++  // If the field is required and empty try to open one of the forms.
++  if (empty($form_state['inline_entity_form'][$ief_id]['entities']) && $instance['required']) {
++    if ($controller->getSetting('allow_existing') && !$controller->getSetting('allow_new')) {
++      $form_state['inline_entity_form'][$ief_id]['form'] = 'ief_add_existing';
++      $hide_cancel = TRUE;

Thank-you for all your continued hard work on this.

sassafrass’s picture

For anyone still looking for a solution, I just discovered the References Dialog module. In combination with the Entity Reference Auto-create and the Autocomplete Widgets modules, it provides a nice interface that allows you to add, edit, or search for existing entities via a pop-up dialog box. https://www.drupal.org/project/references_dialog

netw3rker’s picture

Hi @sassafrass,

You've got a good eye. The patch that you looked at in #89 is definitely in a bad format. It's basically a patch of a patch (which is why you see double plus and minus signs).

This took a lot of careful digging, but I managed to track down the commit it was based on initially and get a re-roll of this to apply to 7.x-1.x (there were conflicts with #0372fbda063627d2403398fddb94e3bafb0a7c6f , #01f569d63528a0800fb8192e14de1c96d15c18a2 and c1f5b47d3204b20556e2da0b3cbddb30eeeb0f03. I made sure these were reasonably/properly resolved.

Good luck getting this through!

devad’s picture

Status: Needs work » Reviewed & tested by the community

I have tested patch #93, and it applies nicely and works nice with latest Inline Entity Form 7.x-1.x-dev.

However, regardless of patch #93, I have tested latest Inline Entity Form 7.x-1.x-dev without patch, and it seems that "Inline entity form - Multiple values" widget works perfect with single valued fields as well. It has "Add existing nodes" button already, so instead of patching "Inline entity form - Single value" widget it would be much better to remove "Inline entity form - Single value" widget all together and keep one widget which works nicely for all use cases.

There is child issue #2290863: Unify the widgets for single and multiple values already, so if this child issue could be resolved I think this issue would become obsolete.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 93: inline_entity_form-allow-to-add-existing-2134035-93.patch, failed testing.

netw3rker’s picture

@devad, I was just working up a write-up saying pretty much the same thing, though I don't think we should get rid of IEF-Single.

This patch appears to make IEF-Single, work just like IEF-Multiple with 1 allowed value.

The IEF - Single, still has advantages. Specifically it lets you start with an open edit form of a selected entity, whereas the ief-multiple doesn't. Also, for IEF-single, it doesn't make sense to have to remove an entity to then add another, it should be one seamless 'switch' option (or two if you count switching to a 'create new' option).

Given that this patch changes the default behavior of IEF-single so dramatically, I think there's going to be a very low likelihood of it being accepted as-is. I'm working on a refactor of this now that I'll post tomorrow evening that is less impactful and provides a more streamlined workflow.

devad’s picture

@netw3rker

Some more brainstorming...

Single, still has advantages. Specifically it lets you start with an open edit form of a selected entity, whereas the ief-multiple doesn't.

Starting with an open edit form of a selected entity option would be nice to have both in single and multiple widgets. Why keeping it for single widget only? Sometimes (in some use cases) multiple widget is there as an option, but in reality 99% of IEF fields are filled with single nodes. So, for such use cases having option for multiple IEF widget to start with open node form could be useful as well.

Also, for IEF-single, it doesn't make sense to have to remove an entity to then add another, it should be one seamless 'switch' option (or two if you count switching to a 'create new' option).

I think current options for removing and adding new or existing nodes in multiple widget are quite practical and good enough for single widget as well.

-----------

From my point of view... it would be nice to rename IEF multiple to just IEF and get rid of IEF single.

It is just a question is it possible to make this change backwards compatible and smooth. But I suppose some nice hook update could do that.

netw3rker’s picture

@devad

Starting with an open edit form of a selected entity option would be nice to have both in single and multiple widgets. Why keeping it for single widget only?

The primary issue here is the ability to work with what currently exists from a functionality perspective. It's unlikely that there will be a new major version of IEF for 7.x, so changing the UI is something that has to be minimally invasive. This means that if IEF-Multiple starts with just the "add existing" and "create new" buttons, It has to keep that. Similarly, the IEF-Single has the expected behavior of having the entity_edit form open and ready to fill out on a field that is IEF-Single, so we can't change this behavior to act like the multiple.

I dig what you are saying about ief-multiple starting with the creation form for an entity though, the main issue is that the nature of its behavior is that there are 2 choices right off the bat, so we'd effectively be arbitrarily deciding which is best for users if we did that. Adding a setting that defaults to the current behavior that can then make it start open though would be a lot more likely to be accepted.

FWIW, I'm still working on this patch - but the current iteration has a "switch [entity]" option that takes you to the default entity handler form and lets you chose from the existing possible entities". It's nice, except that if the target entity has ief entity references, it doesn't load 'em properly. Hopefully I'll have that resolved tonight/tomorrow.

netw3rker’s picture

StatusFileSize
new8.06 KB

Well, better late than never, but here's my stab at the 'switch' field for allowing IEF single and IEF multiple entities to switch to different entities. Now, the 'allow existing' checkbox is available for IEF-Single, and a button is added at the bottom of the Embedded form. Clicking that will allow the user to switch to a different entity. On IEF-Multiple rows, the switch button now appears on each row, and lets the user switch to a new entity directly rather than remove the existing one, and then adding another.

hope this helps!

loze’s picture

netw3rker's patch in #99 works great. Just what I was looking for.
Thanks!

Anonymous’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 99: inline_entity_form-switch_entity.patch, failed testing.

socialnicheguru’s picture

I think this caused an issue:
Notice: Undefined index: #ief_row_delta in inline_entity_form_close_child_forms() (line 1517 of 7/modules/all/inline_entity_form/inline_entity_form.module).

I just used it on a form, entered in the information and clicked save the ief entity.
The entity was added to the entityreference field on the content type but the above error showed up too.

I have not saved the entity yet.

pfrenssen’s picture

@SocialNicheGuru, are you referring to the original patch including test coverage from #93 or the alternative approach from #99?

I've been using #93 and its predecessors in production for two years now and have no problems.

dunebl’s picture

#99 failed to be applied

patching file includes/entity.inline_entity_form.inc
Reversed (or previously applied) patch detected!  Skipping patch.
1 out of 1 hunk ignored -- saving rejects to file includes/entity.inline_entity_form.inc.rej
patching file inline_entity_form.module
Hunk #1 FAILED at 518.
Hunk #2 succeeded at 606 with fuzz 2 (offset 13 lines).
Hunk #3 FAILED at 666.
Hunk #4 FAILED at 909.
Hunk #5 FAILED at 1079.
Hunk #6 FAILED at 1092.
Hunk #7 FAILED at 1111.
Hunk #8 FAILED at 1178.
Hunk #9 FAILED at 1598.
Hunk #10 succeeded at 1643 with fuzz 2 (offset 37 lines).
8 out of 10 hunks FAILED -- saving rejects to file inline_entity_form.module.rej
dunebl’s picture

#99 failed but #93 is doing the job...

dunebl’s picture

FYI: with patch #93 applied, I got the following warning:
asort() expects parameter 1 to be array, null given in /sites/all/modules/inline_entity_form/inline_entity_form.module on line 776

Anonymous’s picture

Status: Needs work » Needs review

#99 still applies just fine to the dev branch as it should.

As for it failing testing - most were from test cases, and I'm not sure if the tests are up to date - setting back to needs review.

dunebl’s picture

@rsmylski : You are right, #99 apply to last dev... sorry for my wrong post!!
It looks like it is working nearly 100% well, I have 2 things to say:
1- I had to change the widget from single value to multiple values to get a change to remove and create a new entity.
2-After having removed and selected an existing entity (not by using the switch entity button, but by removing and after, by adding an existing entity), I saved the form and I got an AJAX error...

dunebl’s picture

FYI, here is the Ajax error I got after clicking on "Add existing XXX"

Chemin : /fr/system/ajax
StatusText: n/a
ResponseText : ?&gt;

But it looks like the added item is really added...

bluegeek9’s picture

Status: Needs review » Closed (outdated)

We appreciate your contributions to Inline Entity Form. Drupal 7 in End-of-Life. We encourage you to upgrade to a supported version of Drupal.

//www.flaticon.com/free-icons/thank-you Thank you for your contribution! Your continued support makes this project sustainable.
There are multiple ways to show appreciation for the work contributed to this project including:

Now that this issue is closed, review the contribution record.

As a contributor, attribute any organization that helped you, or if you volunteered your own time.

Maintainers, credit people who helped resolve this issue.