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.
Now that all entities are converted to entity classes and once #1615236: Merge entity controller interfaces, document and add default entity class definition is on as well, we can remove entity_label(). It's not actually used in core other than in some tests anyway.
Comment | File | Size | Author |
---|---|---|---|
#29 | entity_label.patch | 6.11 KB | fago |
#25 | entity_label-1615240-23.patch | 6.67 KB | webflo |
#24 | entity_label-1615240-22.patch | 6.67 KB | webflo |
#24 | entity_label-22-interdiff.txt | 787 bytes | webflo |
#22 | entity_label.patch | 5.1 KB | fago |
Comments
Comment #1
BerdirHere is a first patch, will fail without #1615236: Merge entity controller interfaces, document and add default entity class definition.
Had to do some funky stuff in the tests, those test entities in field.module should imho be merged with entity_test.module and the entity property test moved to the entity.module as well.
Comment #3
aspilicious CreditAttribution: aspilicious commentedDon't we need the wrapper function stuff for menu callbacks?
Comment #4
BerdirYeah, just read that myself in the other issue.
So it should be renamed (entity_page_label?) and the $entity_type argument removed, we don't need that one anymore. Also, it should just be a wrapper for $entity->label().
Comment #5
fago#4 sounds good to me
Comment #6
BerdirUpdated the patch. Will still fail until the entity controller issue is commited.
Comment #7
BerdirUh, what happened there?
Comment #9
aspilicious CreditAttribution: aspilicious commentedWill need a reroll I guess
Comment #10
Berdir#7: remove-entity-label-1615240-6.patch queued for re-testing.
Comment #11
BerdirNo, not necessary.
Comment #12
aspilicious CreditAttribution: aspilicious commentedIs there a reason for switching to entity_page_label. Whats wrong with entity_label. I don't think it should be renamed because it's depricated in most cases.
Comment #13
fagoIt makes clear it's just there for menu-callbacks. If that needs goes away, entity_page_label() can be obviously removed as well. If we stay with entity_label() that's not so clear as it's supposed to be an API function, not a callback.
Comment #14
aspilicious CreditAttribution: aspilicious commentedI wanted to rtbc but I still don't have a clue why the test additions in field module should be added...
Comment #15
aspilicious CreditAttribution: aspilicious commentedDon't ment to rtbc yet...
Oh well it looks good and Berdir probably will answer my question soon ;)
Comment #16
BerdirThe changes in field_test are necessary because it is required to define a revision table when defining a revision key, that wasn't necessary before because we need to go through entity create for them now.
The current test changes are quite hackish, but that's the easiest way to fix those tests. There is an open issue about moving those tests and the entity types to entity.module/entity_test.module.
Comment #17
fagoyep, having the controller requiring the revision table if revisions are used makes sense.
The cast misses a space. Fixed that and tried to improve the comments of entity_page_label() a bit. Please review.
I guess we should fix drupal_anonymous_user() to return an entity as well? But that can be done in a separate issue.
Comment #18
BerdirWe can't change drupal_anonymous_user(), at least not yet. That function is currently using during session initializing, where the entity system is not yet available.
I have hope that re-factoring the session system to build upon Symfony will allow us to lazy-load the user object, which would make that possible.
Comment #19
BerdirReviewed the changes in fago's patch, they look OK to me, see attached interdiff.
Comment #20
tstoecklerI think it should be: "used as A callback"
Comment #21
webflo CreditAttribution: webflo commentedRerolled and fixed the docs for entity_page_label().
Comment #22
fagoFixed #20 and rerolled the patch.
Comment #23
fagoToo late.. ;)
Comment #24
webflo CreditAttribution: webflo commentedFound entity_label() reference in entity.api.php
Comment #25
webflo CreditAttribution: webflo commentedBetter docs: Entity::label() implements this logic. entity_page_label() not.
Comment #26
fagoGood catch, improvements look great. Back to RTBC then!
Comment #27
aspilicious CreditAttribution: aspilicious commentedIn docs we use full paths, so please change the following. Will leave this rtbc for now.
Should be Drupal\entity\Entity::label()
Drupal\entity\EntityInterface
Same
12 days to next Drupal core point release.
Comment #28
fagoUm true. I don't like it very much as it makes the docs much more verbose, but that's a separate issue.
Comment #29
fagoaddressed #27.
Comment #30
webflo CreditAttribution: webflo commentedLooks good.
Comment #31
webchickOk, I think this makes sense, including the re-naming of the "entity_page_label()" function, since otherwise that looks really tempting to call instead of $entity->label().
I had the same comment about jankiness of drupal_anonymous_user() that was in #17, but this was addressed in the issue as being not possible to clean up now.
Committed and pushed to 8.x. Thanks!
Comment #32
BerdirThis needs a change record, no?
Comment #33
aspilicious CreditAttribution: aspilicious commentedyes it does!
Comment #34
webchickMy mistake, thanks!
Comment #35
BerdirThere are a whole bunch of entity_label() related changes happening right now in various issues:
- Rename it, recommend using $entity->label()
- Add an optional langcode argument.
- Changing core to actually call it instead of e.g. $node->title, $term->name and so on.
- The label callbacks are changed/affected as well (e.g. removal of entity_type), langcode added ...
Not sure if and how we can combine all these changes into change records, make a single one, split it up somehow, ...?
Comment #36
fagoIt's already mentioned that entity_label() is now deprecated in http://drupal.org/node/1400186. Once the entity_uri() patch is in as well, I think we can update this change notice to say they are gone. But as #35 points out there are more entity label related changes, so I think a separate change notice makes sense. (language, callbacks, ..) I'd prefer a single one that summarizes everything related to labels though. That way developers get not overloaded with change notices....
Related, I guess we should think about removing the label callback, but that's another issue.
Comment #37
amaia CreditAttribution: amaia commentedDocumented at http://drupal.org/node/1642962
Comment #38
webchickRestoring original issue properties.
WOOHOO. Thanks for the help, amaia!
Comment #39
amaia CreditAttribution: amaia commentedThank you webchick for walking me through it :)
I hope to be able to contribute some more from now on.
Comment #41
fagoRemoving sprint tag.