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.

Files: 
CommentFileSizeAuthor
#29 entity_label.patch6.11 KBfago
PASSED: [[SimpleTest]]: [MySQL] 36,882 pass(es).
[ View ]
#25 entity_label-1615240-23.patch6.67 KBwebflo
PASSED: [[SimpleTest]]: [MySQL] 36,880 pass(es).
[ View ]
#24 entity_label-1615240-22.patch6.67 KBwebflo
PASSED: [[SimpleTest]]: [MySQL] 36,899 pass(es).
[ View ]
#24 entity_label-22-interdiff.txt787 byteswebflo
#22 entity_label.patch5.1 KBfago
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch entity_label_0.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#21 entity_label-1615240-21.patch5.85 KBwebflo
PASSED: [[SimpleTest]]: [MySQL] 36,888 pass(es).
[ View ]
#19 entity-label-interdiff.txt1.67 KBBerdir
#17 entity_label.patch5.1 KBfago
PASSED: [[SimpleTest]]: [MySQL] 36,824 pass(es).
[ View ]
#7 remove-entity-label-1615240-6.patch5.15 KBBerdir
PASSED: [[SimpleTest]]: [MySQL] 36,807 pass(es).
[ View ]
#1 remove-entity-label-1615240-1.patch4.73 KBBerdir
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details..
[ View ]

Comments

Status:Active» Needs review
StatusFileSize
new4.73 KB
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details..
[ View ]

Here 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.

Status:Needs review» Needs work

The last submitted patch, remove-entity-label-1615240-1.patch, failed testing.

Don't we need the wrapper function stuff for menu callbacks?

Yeah, 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().

Issue tags:+Entity system, +sprint

#4 sounds good to me

Status:Needs work» Fixed

Updated the patch. Will still fail until the entity controller issue is commited.

Status:Fixed» Needs review
StatusFileSize
new5.15 KB
PASSED: [[SimpleTest]]: [MySQL] 36,807 pass(es).
[ View ]

Uh, what happened there?

Status:Needs review» Needs work

The last submitted patch, remove-entity-label-1615240-6.patch, failed testing.

Will need a reroll I guess

Status:Needs work» Needs review

#7: remove-entity-label-1615240-6.patch queued for re-testing.

No, not necessary.

Is 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.

Is 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.

It 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.

Status:Needs review» Reviewed & tested by the community

I wanted to rtbc but I still don't have a clue why the test additions in field module should be added...

Don't ment to rtbc yet...

Oh well it looks good and Berdir probably will answer my question soon ;)

The 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.

Status:Reviewed & tested by the community» Needs review
StatusFileSize
new5.1 KB
PASSED: [[SimpleTest]]: [MySQL] 36,824 pass(es).
[ View ]

yep, having the controller requiring the revision table if revisions are used makes sense.

+++ b/core/modules/user/lib/Drupal/user/Tests/UserEntityCallbacksTest.php
@@ -25,19 +25,19 @@ class UserEntityCallbacksTest extends WebTestBase {
-    $this->anonymous = drupal_anonymous_user();
+    $this->anonymous = entity_create('user', (array)drupal_anonymous_user());

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.

We 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.

Status:Needs review» Reviewed & tested by the community
StatusFileSize
new1.67 KB

Reviewed the changes in fago's patch, they look OK to me, see attached interdiff.

Status:Reviewed & tested by the community» Needs work

+++ b/core/modules/entity/entity.module
@@ -414,31 +415,19 @@ function entity_uri($entity_type, $entity) {
+ * be used as callback, e.g. for menu title callbacks.

I think it should be: "used as A callback"

Status:Needs work» Needs review
StatusFileSize
new5.85 KB
PASSED: [[SimpleTest]]: [MySQL] 36,888 pass(es).
[ View ]

Rerolled and fixed the docs for entity_page_label().

StatusFileSize
new5.1 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch entity_label_0.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Fixed #20 and rerolled the patch.

Too late.. ;)

StatusFileSize
new787 bytes
new6.67 KB
PASSED: [[SimpleTest]]: [MySQL] 36,899 pass(es).
[ View ]

Found entity_label() reference in entity.api.php

StatusFileSize
new6.67 KB
PASSED: [[SimpleTest]]: [MySQL] 36,880 pass(es).
[ View ]

Better docs: Entity::label() implements this logic. entity_page_label() not.

Status:Needs review» Reviewed & tested by the community

Good catch, improvements look great. Back to RTBC then!

In docs we use full paths, so please change the following. Will leave this rtbc for now.

+++ b/core/modules/entity/entity.api.phpundefined
@@ -50,7 +50,7 @@
+ *     entity label. See also the Entity::label() method, which implements this

Should be Drupal\entity\Entity::label()

+++ b/core/modules/entity/entity.moduleundefined
@@ -414,31 +415,19 @@ function entity_uri($entity_type, $entity) {
+ * This is a wrapper for EntityInterface::label(). This function should only

Drupal\entity\EntityInterface

+++ b/core/modules/entity/entity.moduleundefined
@@ -414,31 +415,19 @@ function entity_uri($entity_type, $entity) {
+ * @see EntityInterface::label()

Same

12 days to next Drupal core point release.

Status:Reviewed & tested by the community» Needs work

Um true. I don't like it very much as it makes the docs much more verbose, but that's a separate issue.

Status:Needs work» Needs review
StatusFileSize
new6.11 KB
PASSED: [[SimpleTest]]: [MySQL] 36,882 pass(es).
[ View ]

addressed #27.

Status:Needs review» Reviewed & tested by the community

Looks good.

Status:Reviewed & tested by the community» Fixed

Ok, 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!

Title:Remove entity_label() in favor of EntityInterface::label()Change Notification for: Remove entity_label() in favor of EntityInterface::label()
Priority:Normal» Critical
Status:Fixed» Active

This needs a change record, no?

yes it does!

My mistake, thanks!

There 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, ...?

It'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.

Status:Active» Fixed

Title:Change Notification for: Remove entity_label() in favor of EntityInterface::label()Remove entity_label() in favor of EntityInterface::label()
Priority:Critical» Normal

Restoring original issue properties.

WOOHOO. Thanks for the help, amaia!

Thank you webchick for walking me through it :)
I hope to be able to contribute some more from now on.

Status:Fixed» Closed (fixed)

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

Issue tags:-sprint

Removing sprint tag.