Problem/Motivation

If you have an entity type that has bundles, using _entity_form: $entity_type.add does not work because that ends up calling \Drupal\Core\Entity\EntityForm::getEntityFromRouteMatch() which calls \Drupal\Core\Entity\EntityStorageInterface::create() with an empty array, in particular without the value for the bundle.

Nodes, taxonomy terms, and basically all entity types in contrib (including those using Entity API or Drupal Console) work around this issue by using a dedicated controller instead of the magic _entity_form: $entity_type.add.

However, the bundle value is readily available in the passed route match in getEntityFromRouteMatch() we just have to fetch it before calling EntityStorageInterface::create().

Proposed resolution

Fetch the bundle from the route match in \Drupal\Core\Entity\EntityForm::getEntityFromRouteMatch(). This makes it easier for all entities that have bundles. Also, it will make #2596095: Provide a route provider for add-form of entities a little easier.

Remaining tasks

Contributor tasks needed
Task Novice task? Contributor instructions Complete?
Create a patch Instructions Yes
Add automated tests Instructions Yes
Review patch to ensure that it fixes the issue, stays within scope, is properly documented, and follows coding standards Instructions

User interface changes

None.

API changes

None. Everything that worked before will continue to work. Some things that threw an exception before because of a missing bundle (and thus were worked around, see above) will now magically work. That cannot be considered an API change.

Data model changes

None.

CommentFileSizeAuthor
#17 interdiff.txt1.47 KBbojanz
#17 2651716-17.patch13.4 KBbojanz
#13 2651716-13.patch13.4 KBtstoeckler
#13 2651716-11-13--interdiff.txt933 byteststoeckler
#11 2651716-11.patch13.55 KBtstoeckler
#11 2651716-9-11--interdiff.txt2.98 KBtstoeckler
#9 2651716-9.patch14.04 KBtstoeckler
#9 2651716-5-9--interdiff.txt8.4 KBtstoeckler
#5 2651716-5.patch11.82 KBtstoeckler
#4 2651716-4.patch606 byteststoeckler
#4 2651716-2-4--interdiff.txt606 byteststoeckler
#2 2651716-2.patch11.82 KBtstoeckler
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

tstoeckler created an issue. See original summary.

tstoeckler’s picture

tstoeckler
he/him
Primary language German
Location Essen, Germany
CreditAttribution: tstoeckler as a volunteer commented
Issue summary: View changes
Status: Active » Needs review
FileSize
2651716-2.patch11.82 KB

In addition to the unit test I converted the test entity types to use this new pattern to remove some of the code, so I expect some fallout from that. Let's see.

Status: Needs review » Needs work

The last submitted patch, 2: 2651716-2.patch, failed testing.

tstoeckler’s picture

tstoeckler
he/him
Primary language German
Location Essen, Germany
CreditAttribution: tstoeckler as a volunteer commented
Status: Needs work » Needs review
FileSize
2651716-2-4--interdiff.txt606 bytes
2651716-4.patch606 bytes

Then she met No I.D. and broke his annotation...

tstoeckler’s picture

tstoeckler
he/him
Primary language German
Location Essen, Germany
CreditAttribution: tstoeckler as a volunteer commented
FileSize
2651716-5.patch11.82 KB

...and I'm an idiot. Sorry, testbot, I really didn't mean it!

The last submitted patch, 4: 2651716-4.patch, failed testing.

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented 
+++ b/core/tests/Drupal/Tests/Core/Entity/EntityFormTest.php
@@ -123,4 +129,64 @@ public function testCopyFormValuesToEntity() {
+    $this->assertEquals($entity, $actual);
+
+    // Test an entity type with static bundles.
+    $bundle_key = 'bundle';
...
+    $this->assertEquals($entity, $actual);
+
+    // Test an entity type with a bundle entity type.

Can we split up the tests into several methods?

Status: Needs review » Needs work

The last submitted patch, 5: 2651716-5.patch, failed testing.

tstoeckler’s picture

tstoeckler
he/him
Primary language German
Location Essen, Germany
CreditAttribution: tstoeckler as a volunteer commented
Status: Needs work » Needs review
FileSize
2651716-5-9--interdiff.txt8.4 KB
2651716-9.patch14.04 KB

If there's a prize for dumbest patch roller of the month, I think the decision should be clear by now...

Fixed #7, thanks for the review! I tried to reduce the code duplication between the now-separate methods, I hope that was the direction you were getting at.

Status: Needs review » Needs work

The last submitted patch, 9: 2651716-9.patch, failed testing.

tstoeckler’s picture

tstoeckler
he/him
Primary language German
Location Essen, Germany
CreditAttribution: tstoeckler as a volunteer commented
Status: Needs work » Needs review
FileSize
2651716-9-11--interdiff.txt2.98 KB
2651716-11.patch13.55 KB

Wow, I still think the default value for the bundle is very, very strange, and it would have been nice automatic test coverage, but there are to many fails. Leaving the entity test routing changes as that still covers the new code and is also a nice cleanup.

Status: Needs review » Needs work

The last submitted patch, 11: 2651716-11.patch, failed testing.

tstoeckler’s picture

tstoeckler
he/him
Primary language German
Location Essen, Germany
CreditAttribution: tstoeckler as a volunteer commented
Status: Needs work » Needs review
FileSize
2651716-11-13--interdiff.txt933 bytes
2651716-13.patch13.4 KB

@tstoeckler: FYI, #2596095: Provide a route provider for add-form of entities isn't in yet, in case you didn't notice.

……………………………………..________
………………………………,.-‘”……………….“~.,
………………………..,.-“……………………………..”-.,
…………………….,/………………………………………..”:,
…………………,?………………………………………………\,
………………./…………………………………………………..,}
……………../………………………………………………,:`^`..}
……………/……………………………………………,:”………/
…………..?…..__…………………………………..:`………../
…………./__.(…..”~-,_…………………………,:`………./
………../(_….”~,_……..”~,_………………..,:`…….._/
……….{.._$;_……”=,_…….”-,_…….,.-~-,},.~”;/….}
………..((…..*~_…….”=-._……”;,,./`…./”…………../
…,,,___.\`~,……”~.,………………..`…..}…………../
…………(….`=-,,…….`……………………(……;_,,-”
…………/.`~,……`-………………………….\……/\
………….\`~.*-,……………………………….|,./…..\,__
,,_……….}.>-._\……………………………..|…………..`=~-,
…..`=~-,_\_……`\,……………………………\
……………….`=~-,,.\,………………………….\
…………………………..`:,,………………………`\…………..__
……………………………….`=-,……………….,%`>–==“
…………………………………._\……….._,-%…….`\
……………………………..,<`.._|_,-&“…………….`

tstoeckler’s picture

tstoeckler
he/him
Primary language German
Location Essen, Germany
CreditAttribution: tstoeckler as a volunteer commented

Finally, yay!

So the annotation changes are not actually necessary now to make this green. They are correct, though, and will become necessary in #2596095: Provide a route provider for add-form of entities so I personally wouldn't mind leaving them in. I will of course comply if anyone feels that should be another issue. Same goes for the super minor fix to declaring the test entity type IDs at the right place.

bojanz’s picture

bojanz CreditAttribution: bojanz at Centarro commented

This looks good to me.

Small nitpick:

+        if (($bundle_entity_type_id = $entity_type->getBundleEntityType()) && $route_match->getRawParameter($bundle_entity_type_id)) {
+          $values[$bundle_key] = $route_match->getParameter($bundle_entity_type_id)->id();
+        }
+        if ($route_match->getRawParameter($bundle_key)) {
+          $values[$bundle_key] = $route_match->getParameter($bundle_key);
+        }

The second if could be an elseif.

Otherwise looks RTBC ready.

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner at Chapter Three commented
Status: Needs review » Needs work

Yeah, let's fix #15 and call it a day.

bojanz’s picture

bojanz CreditAttribution: bojanz at Centarro commented
Status: Needs work » Reviewed & tested by the community
FileSize
2651716-17.patch13.4 KB
interdiff.txt1.47 KB

Here we go.

tstoeckler’s picture

tstoeckler
he/him
Primary language German
Location Essen, Germany
CreditAttribution: tstoeckler as a volunteer commented

Yay, thanks! That's a very good point.

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch at Third and Grove commented
Status: Reviewed & tested by the community » Fixed

Looks great and I couldn't find anything to complain about. Committed/pushed to 8.1.x, thanks!

  • catch committed edb618f on 8.1.x 
    Issue #2651716 by tstoeckler, bojanz: Entity::getEntityFromRouteMatch()...
kristiaanvandeneynde’s picture

kristiaanvandeneynde
Primary language Dutch
Location Antwerp, Belgium
CreditAttribution: kristiaanvandeneynde at Deeson commented

Very nice addition to Entity API, well done!

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.

fgm’s picture

fgm
Primary language French
Location Paris, France
CreditAttribution: fgm at OSInet for Klee Interactive (Klee Group) commented
Related issues: +#2743303: EntityForm::getEntityFromRouteMatch() does not support route-level parameters

Added followup issue about route defaults at #2743303: EntityForm::getEntityFromRouteMatch() does not support route-level parameters