Problem/Motivation
#2596095: Provide a route provider for add-form of entities was refocused to only add the add-form route.
But any content entity with bundles also needs an add-page, that will allow the user to select a bundle and be sent to the form.
This is generic code and doesn't need to be repeated.
We need to extend the route provider, add a controller method, and add an EntityDescriptionInterface (contrib example) for $bundle_entity->getDescription().
Contrib implementation:
https://github.com/fago/entity/blob/8.x-1.x/src/Routing/CreateHtmlRouteP...
https://github.com/fago/entity/blob/8.x-1.x/src/Controller/EntityCreateC...
https://github.com/fago/entity/blob/8.x-1.x/templates/entity-add-list.ht...
Proposed resolution
Let's add it.
Remaining tasks
User interface changes
API changes
Data model changes
Comment | File | Size | Author |
---|---|---|---|
#50 | 2666792-50-add-page-follow-up.patch | 1.37 KB | kristiaanvandeneynde |
#48 | 2666792-48-add-page-follow-up.patch | 1.37 KB | kristiaanvandeneynde |
#47 | 2666792-47-add-page-follow-up.patch | 3.19 KB | kristiaanvandeneynde |
#38 | interdiff.txt | 6.34 KB | bojanz |
#38 | 2666792-38-add-page.patch | 32.21 KB | bojanz |
Comments
Comment #2
dawehner.
Comment #3
bojanz CreditAttribution: bojanz at Centarro commentedAdded the interface, made it used by NodeType (note that there wasn't a setter before. And for some reason no other setters return $this.)
Comment #4
dawehnerMh, so this is sort of a BC break on
NodeTypeInterface
, but I don't think someone actually implements that again.Comment #5
bojanz CreditAttribution: bojanz at Centarro commentedWorst case scenario, we skip updating NodeTypeInterface, make this for new code only.
Comment #6
dawehnerYeah no idea. We could also just let NodeType implement the interface, so on runtime you can still check for
EntityDescriptionInterface
Comment #7
bojanz CreditAttribution: bojanz at Centarro commentedI spoke to berdir. The official position is not to extend existing interfaces. For example: https://www.drupal.org/node/306662#comment-10816832
So let's leave the core conversion battle for a different issue. We can refocus this issue on adding the interface and using it for the add-page route, since that part was removed from #2596095: Provide a route provider for add-form of entities
Comment #8
bojanz CreditAttribution: bojanz at Centarro commentedComment #9
dawehnerRevert the title change ...
Comment #10
dawehnerThere we go.
Comment #12
dawehnerTake it bojanz!
Comment #14
bojanz CreditAttribution: bojanz at Centarro commentedInital stab at at it. Still needs the test from https://github.com/fago/entity/blob/8.x-1.x/tests/src/Functional/CreateU...
Comment #16
bojanz CreditAttribution: bojanz at Centarro commentedAdded missing stable template override.
Comment #17
heddnThe twig template here seems identical to the one used in node module. But the interesting thing is that the node-add-list.html.twig is then overridden in Seven theme so it looks pretty. But this one looks really ugly. Could we use the Seven style twig template instead of the node module's? Screenshots and updated patch to follow.
Comment #18
heddnThis returns a ConfigEntityType in my local setup. We need EntityDescriptionInterface to loadBundleDescriptions() down in line 149.
This needs to be a EntityDescriptionInterface, but it isn't.
Comment #19
edysmpThis updates the twig template.
Comment #20
edysmpBefore:
After:
Note that in the after, the description still doesn't appear.
Comment #21
bojanz CreditAttribution: bojanz at Centarro commentedThanks, edysmp!
The description won't appear until bundle config entity types (such as NodeType) implement the EntityDescriptionInterface. That's normal.
Comment #22
dawehnerWorking on some integration test coverage now.
Comment #23
dawehnerI moved this special template from stable to seven. I'm not convinced of nesting div elements into a
a
element, but yeah this is what is done for node as well.Comment #25
dawehner:(
Comment #27
heddnre #23: should we still leave a basic template in the system module? As to copy node/seven more closely?
Comment #28
heddnIgnore me. I see we do have a copy in the system folder.
Comment #29
bojanz CreditAttribution: bojanz at Centarro commentedI have once again added an exact copy of entity-add-list.html.twig to the Stable theme, since there's a test that checks whether each module template has a matching Stable template.
Also reworked the logic that distinguishes between entity and non-entity bundles.
We now have green entity bundle tests, but we also need to test non-entity bundles.
Comment #30
bojanz CreditAttribution: bojanz at Centarro commentedComment #31
dawehnerThe non bundle entity type fallback is looking pretty good.
It is still weird to do that, to be honest. For example for 'user' this would mean that this page is uncached, everytime a user joins/updates itself. This doesn't really make sense for me at least.
Comment #32
bojanz CreditAttribution: bojanz at Centarro commentedYou're right, removed it.
Also fixed a notice, and added the test coverage for non-entity bundles.
Comment #33
dawehnerNice, that this was actually not used by something.
Comment #34
bojanz CreditAttribution: bojanz at Centarro commentedForgot to sync the seven template. Now done.
Comment #35
catchGenerally looks good, only the last point is substantial at all:
ID.
Why isSubClassOf and not instanceof?
Why is this done in preprocess and not in EntityController::addPage()?
Comment #36
bojanz CreditAttribution: bojanz at Centarro commentedWe are checking whether the entity class implements the interface, not the entity type.
isSubclassOf() is still the main way of doing that, no?
Mostly because template_preprocess_node_add_list() does something similar, and the approach carried over.
I would keep the #markup part in the preprocess.
It allows the following part of the docblock to stay true (and simple):
I'll move the add_bundle_message logic to EntityController::addPage().
Comment #37
BerdirWe're considering to deprecate isSubclassOf, it has caused confusions like this multiple times, see #2194783: Rename EntityTypeInterface::isSubclassOf() to ::entityClassImplements().
Comment #38
bojanz CreditAttribution: bojanz at Centarro commentedReplaced "id" with "ID", moved the add_bundle_message logic to the controller, tweaked comments.
I can replace isSubclassOf() if you wish, but to me it's just an uglier version of is_subclass_of($entity_type->getClass(), '')...
Comment #39
BerdirI guess you meant to say a nicer version of? :)
Yes it is, but the problem with it is that it's quite confusing, I've seen that feedback/question from reviewers in quite a few issues. And if we're really going go deprecate it, then it's one call less to worry about.
Comment #40
dawehnerYeah I totally believe arguing about that is out of scope of this issue.
Comment #41
catchOK I hadn't seen #2194783: Rename EntityTypeInterface::isSubclassOf() to ::entityClassImplements() but clearly I got confused the same as other people have, so following that issue now. Agreed it's out of scope here and sorry for the distraction.
Committed/pushed to 8.2.x and cherry-picked to 8.1.x. Thanks!
Comment #42
catchComment #43
kristiaanvandeneyndeThe edit-form and delete-form paths seem a bit weird? Normally you find "manage" in the path for config entities and the delete-form path just looks weird altogether :)
Also, semi-on-topic: I can't find add-page (nor add-form) in http://www.iana.org/assignments/link-relations/link-relations.xhtml, only create-form. Shouldn't we prefix that with "drupal:" like we do in other places in core? I am having a hard time understanding why we sometimes prefix them and sometimes we don't.
Other than that I'm really excited about this patch. It also finally adds a test for bundle entities! That one had been blocking #2645202: Re-saving a new bundle entity throws an exception for a while.
I guess I'll have to reroll that one's tests because I assume this one will go in first.Edit: It got committed while I was reviewing :o
Comment #44
bojanz CreditAttribution: bojanz at Centarro commented@kristiaanvandeneynde
We have no rule that '/manage/' should be present in the urls, especially for test entities, right?
The delete-form url definitely looks wrong (though it will work).
Comment #45
kristiaanvandeneynde@bojanz I was pointing out that it's in there, not that it should be in there :)
Those two should probably look like:
Comment #46
bojanz CreditAttribution: bojanz at Centarro commented@kristiaanvandeneynde
Ah, you're right. We should fix that in a followup.
Comment #47
kristiaanvandeneyndeFixed a few trailing commas while I was at it. I recall reading somewhere YAML doesn't like that?
Comment #48
kristiaanvandeneyndebojanz just told me Drupal fixed the issue with trailing commas, so new patch.
Comment #49
dawehnerHaving the comma here would still be great
Comment #50
kristiaanvandeneyndeUgh, was trying to be too fast.
Comment #52
dawehnerNo worries
Comment #53
catchFollow-up makes sense, but please post it to a new issue linked from here.
Comment #54
catchMeh it's just removing two words. I'll commit if it comes back green from the bot, if not new issue please though.
Comment #55
catchCommitted/pushed to 8.2.x and cherry-picked to 8.1.x. Thanks!
Comment #59
xjmThis issue is missing a change record. (We should add change records for noteworthy API additions.)
Comment #60
xjmWe can probably create a combined CR for this and #2596095: Provide a route provider for add-form of entities.
Comment #61
dawehnerI'll write one for all of the 4 entity related ones: https://www.drupal.org/node/2709511
Comment #62
dawehnerAdded stuff to the change record. Feel free to improve it.