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.
Posted by xjm
This issue is a followup for #1781372: Add an API for listing (configuration) entities and #1783964: Allow entity types to provide menu items.
Problem/Motivation
- In Drupal 7, when no entities of a certain type are defined, a listing of those entities
theme_table()
typically has a link to add one in the table's empty text. - The entity list controller in #1781372: Add an API for listing (configuration) entities currently does not provide this functionality.
- In order for an entity list controller to provide this link, it needs to know its path. (See #1781372-102: Add an API for listing (configuration) entities for details.)
- #1783964: Allow entity types to provide menu items discusses how entities or controllers can define their paths.
Proposed resolution
- Add back an "Add" link inside an empty list table once #1783964: Allow entity types to provide menu items is resolved.
Remaining tasks
- #1783964: Allow entity types to provide menu items probably blocks this issue.
User interface changes
Current behavior
Intended behavior
Comment | File | Size | Author |
---|---|---|---|
#120 | rerolldiff_110-120.txt | 43.4 KB | ayush.khare |
#120 | 1798540-120.patch | 61.75 KB | ayush.khare |
#110 | 1798540-110.patch | 61.45 KB | jofitz |
#106 | interdiff-1798540-104-106.txt | 17 KB | martin107 |
#106 | 1798540-106.patch | 63.81 KB | martin107 |
Comments
Comment #1
xjmComment #2
xjmComment #3
xjmComment #3.0
xjmUpdated issue summary.
Comment #4
dawehnerI'm wondering whether this should be part of the interface, so i started without it.
It feels ugly to remove all existing config entities in the test to be able to check this
functionality.
Comment #5
sunHrm. We really need an entity operations/actions API. That has become crystal clear through a dozen of issues already.
I rather object to working around that missing puzzle piece even further. Let's fix the root cause/limitation instead.
An entity operations/actions API would inherently depend on #1696660: Add an entity access API for single entity access, since that introduces generic access handling, so each operation/action can be properly checked for access before it is exposed.
Comment #6
andypostClosely related #1839516: Introduce entity operation providers
The "Add item/sub-item" could be a operations
Comment #7
sunThe proposed change here attempts to hardcode a 1:1 relationship between an entity and where it appears in a listing, or perhaps rather, where its "Add form" is located.
However, architecturally, that's really not where we want to go. In case you know http://drupal.org/project/context_admin, that's much more in line with our architectural goals — entities can be listed in various contexts, and within each context, you are able to add a new, but also manage existing.
In a bit more abstract RESTful speak, this means:
Unfortunately, the pure RESTful pattern cannot be applied to web-based form interaction. A typical way to work around it would be to replace the {id} with a static placeholder string that would be invalid as a regular/true ID otherwise; e.g.:
In turn, regardless of the current context, you're able to retrieve the URI for creating/adding a new entity based on its regular URI pattern:
I'm aware that we didn't make much progress on contexts yet, but the proposed change here with a hard 1:1 binding would implicitly move us into a direction that is not aware of contexts, whereas we want to go into the opposite direction and make this stuff context-aware.
Tagging with Blocks/Layouts, in the hope that @EclipseGc and others might be able to chime in.
Comment #8
jibran#4: drupal-1798540-4.patch queued for re-testing.
Comment #10
andypostI think all entities should have uri() method implemented or use parent
Comment #11
dawehnerWell, but how can we use uri() to create a link to a creation for of entities? I don't see how this can workout.
Comment #11.0
dawehnerTypo.
Comment #12
damiankloip CreditAttribution: damiankloip commentedYeah, uri() won't really help us as we don't have an entity. Also I thought #1970360: Entities should define URI templates and standard links may have helped us here, but it seems that the summary mentions create urls but it didn't make it into the patch?
Comment #12.0
damiankloip CreditAttribution: damiankloip commentedRemoving myself from the author field so I can unfollow. --xjm
Comment #13
jamesquinton CreditAttribution: jamesquinton commentedRe-rolled patch and updated.
Comment #14
jamesquinton CreditAttribution: jamesquinton commentedWoops..! Here's the correct patch.
Comment #17
jamesquinton CreditAttribution: jamesquinton commentedUpdated and added link to the following lists:
I don't think every entity list needs this, for example User Roles - where you can’t remove the default roles, and therefore never have an empty list.
“entity-add-form” link key could probably be named better.
Comment #18
jamesquinton CreditAttribution: jamesquinton commentedDoh - rerolling without my .gitignore in the patch...
Comment #19
jamesquinton CreditAttribution: jamesquinton commentedRerolled
Comment #20
dawehnerThank you!
A few more words would be cool
Let's call it just url or url_object.
Comment #21
jamesquinton CreditAttribution: jamesquinton commentedCool - agreed!
Here's a new patch with your suggestions implemented...
Comment #22
damiankloip CreditAttribution: damiankloip commentedadd-entity-form seems to break the pattern of
$action-form
.Comment #23
tim.plunkettAt NYCCamp we had a long discussion about the naming, and the weirdness of NodeType having a route for node.add and node.type_add, and what to name them.
If we really care about the naming, we should open a dedicated issue, because I'm pretty sure the existing
"add-form" = "node.add",
pattern is wrong.Awesome work @jamesquinton, (hopefully) congrats on your first core patch!
Comment #24
damiankloip CreditAttribution: damiankloip commentedIMO having the actual node type as add-entity-form is even more confusing. So add-form does is not for the actual entity type of the annotation it's on but for nodes, whereas add-entity-form is for the current entity. Maybe at least add-type-form?
Looks like this was happening before this, but why are node and node type mixing the link templates so much? This just seems wrong.
Comment #25
tstoecklerSo the reason why "node.add_form" (i.e. node/add/{type}) is on
NodeType
instead ofNode
is because it has the node type as a route parameter and not a node. I also think that is fairly weird but I also have no clue how to fix this.Comment #26
damiankloip CreditAttribution: damiankloip commented:/
Comment #27
jamesquinton CreditAttribution: jamesquinton commentedUnasigning myself as I think this is now more of a naming issue now...
I do agree that it seems strange for node.add_form to be on NodeType instead of Node.
Interestingly, custom blocks are kind of similar - but with Custom Block Types, there is "custom_block.type_add".
Comment #29
jamesquinton CreditAttribution: jamesquinton commentedComment #30
jamesquinton CreditAttribution: jamesquinton commentedRe-rolled patch.
Comment #31
Xanoadd-entity-form
toadd
, since it's obvious that this is about entities, and the route may not point to form pages. The one for nodes, for instance, points to a page where users can select the type of which they want to add a node.EntityAccessControllerInterface
a little.EntityListBuilder
.Comment #34
XanoComment #36
XanoMy apologies for not reading the issue thoroughly enough and explaining why I made some changes. Here's my reasoning
add-entity-form
, though.EntityAccessControllerInterface
change from last patch. It does not break any existing code, but it may not work properly with existing code either. It does allow us to check access to theadd-entity-form
link template's route for entity types that have bundles, though.This patch reverts the link template name, and fixes some test failures.
@jamesquinton: I learned that this is your first core issue and you started on it as part of a sprint. I tried to make this patch a starting point for you to finish fixing the test failures and learn something about dependency injection. See the interdiff for how some of the previous failures were fixed. If you are no longer interested or cannot finish this issue, that is fine and I will do it. Otherwise, the honor is yours and I will review your patches.
Comment #37
jamesquinton CreditAttribution: jamesquinton commented@Xano - thanks for the detailed explanation! Definitely still interested, I'll take a look (hopefully today)...
Comment #38
XanoCool!
I forgot to trigger the bot. Let's see how much fails.
Comment #40
jamesquinton CreditAttribution: jamesquinton commentedOK - I'm having trouble running tests locally at the moment, but hopefully this should reduce the number of errors...
Comment #41
jamesquinton CreditAttribution: jamesquinton commentedComment #43
jamesquinton CreditAttribution: jamesquinton commentedComment #44
jamesquinton CreditAttribution: jamesquinton commentedI'm starting to think this may be better suited to someone who is more comfortable with dependency injections, as this is still a relatively new concept to me!
I think I will have to bail out of this one, at least until I am a bit more familiar with dependency injections...
Comment #45
tim.plunkettI think this has gotten a little out of hand with the DI, I don't blame you for being confused @jamesquinton. I'm going to try and get this back on track.
Comment #46
XanoUnassigning, since Tim just left the sprint and is on a plane.
This patch removes the dependency injection.
Comment #47
Xano46: drupal_1798540_46.patch queued for re-testing.
Comment #49
XanoRe-roll.
Comment #50
dawehnerDid we thought about not using a dot at the end? Did we considered to put @label of $this->enttityType->getLabel() in there?
I can haz injections or at least helper methods?
This looks wrong.
Comment #51
XanoYes. See the overrides in the other classes. These are sensible defaults that are grammatically and stylistically correct, as we can't dynamically do that by using entity type names.
It was decided that that's for a follow-up and we focus on just adding the link here.
Comment #52
dawehnerYeah, all people know that you are lazy.
Comment #53
tim.plunkettThis change is just wrong.
Comment #56
dawehnerso.
Comment #59
tim.plunkettNot actively part of the Blocks-Layouts work.
Comment #61
pfrenssenI tried to rebase this onto 8.4.x for the third anniversary of the patch but the code base has changed dramatically in the meantime. It seems like it will be simpler to reroll it from scratch.
Comment #62
pfrenssenI went through the comments on this issue and at the time there was a lot of confusion about the link template to use. The patch settled on
add-entity-form
but this was still being debated.In the meantime this has been cleared up, there is an
add-page
link template (ref.DefaultHtmlRouteProvider::getAddPageRoute()
) for entity types that have bundles, leading to a page where a bundle can be chosen for the entity to create, such asnode/add
. There is also anadd-form
route (ref.DefaultHtmlRouteProvider::getAddFormRoute()
) which leads directly to the create form. It requires a bundle parameter if the entity type has bundles.It does seem that there is no wide support for
add-page
andadd-form
yet but this is trivial to add. This can be done in this issue as was done in the original patch.In the entity list builder we can check if the entity has bundles and point to
add-page
if it is bundlified, andadd-form
if it isn't.I also reviewed the patch and from a DX standpoint I think it's great to add a
getEmptyText()
method. This allows developers to easily control the exact text that is shown for their entities. It's not so great though that the final composition of the empty text is still handled in therender()
function, so ultimately the developer _doesn't_ have control over the text. Let's do the concatenation of the two strings insidegetEmptyText()
.Comment #63
joachim CreditAttribution: joachim as a volunteer commentedI think #62 sounds like a good plan except for this point:
> In the entity list builder we can check if the entity has bundles and point to add-page if it is bundlified, and add-form if it isn't.
An entity type might not be using DefaultHtmlRouteProvider, and might not be following the conventions for link templates that are used there. So rather than check for the entity type using bundles, I think we should check the link templates. In pseudocode, the logic would be:
Comment #64
pfrenssen@joachim, thanks, that sounds like the best approach!
Started the work on the reroll. No interdiff since this is starting again from scratch, the source tree has diverged too much to make a rebase feasible. This is not yet finished, so not burdening the bot with it.
Comment #65
pk188 CreditAttribution: pk188 at OpenSense Labs commentedComment #67
pk188 CreditAttribution: pk188 at OpenSense Labs commentedDid some changes. Please check once.
Comment #69
jofitz CreditAttribution: jofitz at ComputerMinds commented@pk188 Be aware that this is a work in process containing psuedo-code, hence why @pfrenssen said in #64:
Comment #71
pfrenssenRerolled to 8.5.x, converted the pseudocode into PHP, and addressed the @todo's in the code. This is completely untested yet.
Comment #72
pfrenssenJust setting to needs review to get test feedback from the bot. This still needs work.
Comment #73
pfrenssenAdded some of the missing link templates. It's a lot of work actually to check all existing entity types and hunt for their forms.
Comment #76
pfrenssenAddressing test failures.
Comment #78
pfrenssenFixing a few more failures.
Some of the new link templates have legacy route names that predate the structured entity paths and are not discovered automatically. It appears that the Symfony router does not support aliases for routes so I am overriding the EntityInterface::toUrl() method instead.
Comment #80
pfrenssenFixing another test. Still need to figure out where the REST tests get their expected data from.
Comment #83
pfrenssenThis needed a reroll because of #2942588: EntityListBuilder::render() should use the entity type plural label in the empty text.
Comment #84
pfrenssenAnother reroll.
Comment #85
joachim CreditAttribution: joachim as a volunteer commentedIs there any benefit to returning FALSE instead of NULL? NULL is easier to read, as the return statements are simpler.
You can then also use isset() instead of !empty() for the condition that checks the return, which is easier to read as it's not a double negative.
Typo. Should be = not :
Same here.
Comment #86
pfrenssenThanks for the review, Joachim! Fixed the remarks.
Comment #88
pfrenssenStraight reroll.
Comment #90
pfrenssenFixed failures regarding the BlockContent entity.
Comment #92
martin107 CreditAttribution: martin107 as a volunteer commentedAnother common point of failure is RoleListBuilder
Comment #94
martin107 CreditAttribution: martin107 as a volunteer commentedmore commas,
Comment #96
martin107 CreditAttribution: martin107 as a volunteer commentedLess errors.
Comment #98
martin107 CreditAttribution: martin107 as a volunteer commentedA) fixed up MediaListBuilder::construct()
B) As this patch adds new test code it should use Object:class strings as the error reporting will be sharper, more semantically meaningfull when changes elsewhere introduce failures.
Comment #100
martin107 CreditAttribution: martin107 as a volunteer commentedWorkspaceListBuilder.
Comment #102
martin107 CreditAttribution: martin107 as a volunteer commentedJust sweeping up some minor coding standard errors...
We don't need to introduce a stray use of
Drupal\Component\Utility\Unicode;
Comment #104
martin107 CreditAttribution: martin107 as a volunteer commentedI want rid of a small inconsistency
code we are introducing in EntityListBuilder:getAddLink() introduces a hidden dependancy on the LinkGenerator.
Fixing this small conceptual change .. ripples through 23 other files.
Comment #106
martin107 CreditAttribution: martin107 as a volunteer commentedIn #104
I systematically added a coding standard error regarding @param ... link_generator.
and introduced a error..my bad
Comment #109
martin107 CreditAttribution: martin107 as a volunteer commentedNeeds reroll.
Comment #110
jofitz CreditAttribution: jofitz at ComputerMinds commentedRe-rolled.
Comment #120
ayush.khare CreditAttribution: ayush.khare at OpenSense Labs for DrupalFit commentedReroll #110 for 10.1.x
Comment #121
larowlanIs the disruption of injecting the link generator worth it?
All the other links we generate in this class (e.g. in operations) don't need/use it so that makes me feel we may not need it.
That would dramatically reduce the complexity of this patch and the disruption to contrib.
Is internal: our only option here?
\Drupal\Core\Entity\Controller\EntityController::addPage
usesLink::createFromRoute
This could return an exception if there is no add page link template, but there is an add-form link template but it requires additional parameters. So we probably would need a try/catch here.
Also, we don't need elseif, there's a return above.