Replace $node->title with $node->label()

I'm working on this.

I attach a patch that solves this

Patch that solves the test bot error

This will be the correct one? ;)

Huge patch :) Looks good tough on a quick look-through.

I attach a patch that pass the tests and have the corrections made by fago.

@fago, can you check my new patch and make me some suggestions about the use of $node->title or $node->label()? I don't understand very well your previuos comment

Fixed problems when applying the patch

Your patch conflicted with the system.test file. These tests are moved to another location and you forgot to fix those files. Thats why your new patch is smaller.

Still needs wok then?

I found a few more usages of the title property (mostly in l() invocations and rdf). I did a regex against "node" and "title" in the same line and added everything (apart from the token stuff) that made sense I believe. I might have missed usages of the title attribute in loops where $item is used or in other cases though.

@fubhy There are some usages of $node->title that refers to the result of a query. In this cases we can't replace $node->title with $node->label() because it's a query result object instead of an entity object. I'm fixing this little problems. In short I'll attach a new patch

Okay, I didn't think of that - It was more or less a blind regex. :)

I hope that it helped to identify a few leftover occurrences of the title property anyhow.

Rerolled to take into account data coming from the DB.

I think you uploaded some kind of strange interdif :)

Indeed, here is the correct patch.

/me crosses fingers.

On #1616972: Replace $term->name with $term->label(), the latest patch leaves the term token "name" with the $term->name value, whereas the patch on #25 replaces the node "title" token with the $node->label() value.
We should probably make the same choice everywhere.
We can :
1) leave the old tokens and do not create new ones for $term->label() and $node->label().
2) leave the old tokens and create new ones for $term->label() and $node->label().
3) keep the old token names but replace their value with $term->label() and $node->label().

I created a follow-up on label() related tokens for $term and $node to continue this discussion : #1632720: Create tokens for entity labels.

In the meantime i think we should take the token changes out of the current patch for this issue.

#25: node-title-label-1616962-24.patch queued for re-testing.

The test in #25 passes locally and the error happens in setUp() which the change didn't touch, so it looks like it could be a qa bot fluke, so retesting...

Rerolled without the term->label() change in forum admin.

Here is for the rerolled patch.

Rerolled after yesternight's commits, just in case they broke something

+++ b/core/includes/database.incundefined
@@ -44,7 +44,7 @@ use Drupal\Core\Database\Query\Condition;
  * foreach ($result as $record) {
- *   // Perform operations on $node->title, etc. here.
+ *   // Perform operations on $node->label(), etc. here.

The context here is hard to see, but this might actually be wrong. Because it talks about performing operations, which implies changing something and you can't change title().

+++ b/core/modules/book/lib/Drupal/book/Tests/BookTest.phpundefined
@@ -142,7 +142,7 @@ class BookTest extends WebTestBase {
-      $this->assertRaw(l('<b>‹</b> ' . $previous->title, 'node/' . $previous->nid, array('html'=> TRUE, 'attributes' => array('rel' => array('prev'), 'title' => t('Go to previous page')))), t('Previous page link found.'));
+      $this->assertRaw(l('<b>‹</b> ' . $previous->label(), 'node/' . $previous->nid, array('html'=> TRUE, 'attributes' => array('rel' => array('prev'), 'title' => t('Go to previous page')))), t('Previous page link found.'));

Usually, the rule is to fix CS on changed lines. We probably don't want to do this on a page of this size. Just wanted to note that there is a space missing after html.

+++ b/core/modules/forum/forum.moduleundefined
@@ -714,7 +714,11 @@ function forum_block_view_pre_render($elements) {
-    $elements['forum_more'] = array('#theme' => 'more_link', '#url' => 'forum', '#title' => t('Read the latest forum topics.'));
+    $elements['forum_more'] = array(
+      '#theme' => 'more_link',
+      '#url' => 'forum',
+      '#title' => t('Read the latest forum topics.'),

How is this change related?

+++ b/core/modules/poll/poll.pages.incundefined
@@ -1,4 +1,4 @@
-<?php
+  <?php

Accidental space in here.

+++ b/core/modules/poll/poll.pages.incundefined
@@ -37,6 +37,7 @@ function poll_page() {
   $output = '<ul>';
+  // We cannot use $node->label() here because $node comes straight from the DB.
   foreach ($queried_nodes as $node) {
     $output .= '<li>' . l($node->title, "node/$node->nid") . ' - ' . format_plural($node->votes, '1 vote', '@count votes') . ' - ' . ($node->active ? t('open') : t('closed')) . '</li>';

Can we open a follow-up for this? Loading nodes directly through queries is wrong, can lead to errors (trying to pass it to functions that now require a class) and should be fixed. Also for those below.

Also, as pointed out in #1616972: Replace $term->name with $term->label() #13 and #19, we need to make sure that we only use label when we intend to print it out somehow. If we for example compare it (e.g. some test hooks), we should probably stick with node->title.

Follow up done at #1637358: poll.module should not query the node table directly.

Rerolled for #33. forum_block_view_pre_render() was mentioned in the #30 diff, but is indeed out of scope for this.

+++ b/core/modules/node/node.tokens.incundefined
@@ -130,7 +130,7 @@ function node_tokens($type, $tokens, array $data = array(), array $options = arr
         case 'title':
-          $replacements[$original] = $sanitize ? check_plain($node->title) : $node->title;
+          $replacements[$original] = $sanitize ? check_plain($node->label()) : $node->label();

According to #30, this shouldn't be changed. It makes sense for comment:node, because we can safely assume that we want the label there.

+++ b/core/modules/node/tests/modules/node_test/node_test.moduleundefined
@@ -126,15 +126,15 @@ function node_test_node_grants_alter(&$grants, $account, $op) {
 function node_test_node_presave(Node $node) {
-  if ($node->title == 'testing_node_presave') {
+  if ($node->label() == 'testing_node_presave') {

I think this one should still reference to the title, as mentioned above.

+++ b/core/modules/node/tests/modules/node_test/node_test.moduleundefined
@@ -126,15 +126,15 @@ function node_test_node_grants_alter(&$grants, $account, $op) {
   // Determine changes.
-  if (!empty($node->original) && $node->original->title == 'test_changes') {
-    if ($node->original->title != $node->title) {
+  if (!empty($node->original) && $node->original->label() == 'test_changes') {
+    if ($node->original->label() != $node->label()) {
       $node->title .= '_presave';

@@ -145,8 +145,8 @@ function node_test_node_presave(Node $node) {
   // Determine changes on update.
-  if (!empty($node->original) && $node->original->title == 'test_changes') {
-    if ($node->original->title != $node->title) {
+  if (!empty($node->original) && $node->original->label() == 'test_changes') {
+    if ($node->original->label() != $node->label()) {
       $node->title .= '_update';

+++ b/core/modules/node/tests/modules/node_test_exception/node_test_exception.moduleundefined
@@ -12,7 +12,7 @@ use Drupal\node\Node;
 function node_test_exception_node_insert(Node $node) {
-  if ($node->title == 'testing_transaction_exception') {
+  if ($node->label() == 'testing_transaction_exception') {
     throw new Exception('Test exception for rollback.');

Same.

+++ b/core/modules/tracker/tracker.pages.incundefined
@@ -70,6 +70,7 @@ function tracker_page($account = NULL, $set_title = FALSE) {
+         // Do not use $node->label(), $node comes straight from the DB.

We usually don't abbreviate, so this should be database.

We should keep track of all these cases, not just poll. Not sure if we want to open separate issues or a single meta issue that just lists all cases for now. As far as I can see, there is one in statistics, search and tracker (+ poll).

Added the note about the other node queries in the other issue.

Patch rerolled.

+++ b/core/modules/node/node.moduleundefined
@@ -2010,7 +2011,7 @@ function node_type_page_title($type) {
 function node_page_title(Node $node) {
-  return $node->title;
+  return $node->label();

Lets get rid of node_page_title() and use it with entity_page_label() in a follow-up issue. Otherwise its good.

will fix this

Followup to label()-based tokens in addition to title is #1632720: Create tokens for entity labels.

Rerolled patch. (Tests pass locally for node and translation).

looks good for me, as we found out it has still some missing things like in the forum, where not $node->title is used

But as @Berdir said in #1616972: Replace $term->name with $term->label() I would create a follow up, how we handle these changes in template files.

That would probably be especially true now that we will be rewriting all templates for twig anyway.

Followup in #1642070: Make use of entity labels in templates.

Rerolled accordingly.

Maybe we want to introduce a %label in system.api.php#hook_mail() to show that this method can be used when composing mails as an alternative to the raw title ?

Wow, quite the work! Thanks. Benchmarks? Do we need them?

#46: 0001-Issue-1616962-replace-node-title-with-node-label.patch queued for re-testing.

@chx: Because the label will be multilingual, you either need an accessor method or magic getter/setter for it. Since we are directly using the method here, not even a magic getter, not sure how could we get the same functionality with better performance. Any ideas?

I attached a newer version of the patch.

Fixing typo.

Straight reroll.

Just asking, but does this need some benchmarks, since we're going from a straight property reference to a method call?

See Gabor in #50. Directly accessing a property is not an option if we want to allow translated labels. Or actually be able to properly rely on alterable labels, which was introduced in 7.x but is quite pointless there because entity_label() isn't used.

For example, user names currently have a second API (format_username(7.x)/user_format_name(8.x)) that we can get rid of once user entities are converted as well.

For the id() methods, we decided to remove the magic and require an explicit, hard-wired implementation for each entity class that doesn't use the default id property because that is the fastest possible way and we assume that that function is going to be called a lot. I don't think label() is going to be called as often nor is it possible to actually hardcode something there.

So yes, calling a method with some magic in it is slower, but it's unavoidable I think.

#54: 1616962-54.patch queued for re-testing.

If there is any problem, I can dedicate time so solve it.

Ok, the patch itself is very easy and simply swaps $node->title for $node->label() everywhere. ->label() comes from EntityInterface, and Node is inheriting its logic from Entity::label(). It sounds like there's not a way of doing what we need with pure properties, so we need to eat the performance impact regardless, so I am fine committing this patch. However, I don't think catch would approve of doing so without at least documenting the performance impact, so if someone could please run before/after xhprof numbers, that would be hugely appreciated.

The question I raised in IRC is whether we should remove the $node->title property, since that still exists when you var_dump($node) from, say, node.tpl.php, which raises the likelihood that people will just use that property directly and not go through the API. However, berdir pointed out that this still needs to exist because sometimes (for example on node edit forms) you need to get the "raw" value of the title for presenting to the user. So we're going to keep it for now, but we'll need some sort of holistic answer to this going forward throughout all entity methods, since var_dump($entity) doesn't actually let you know they're there.

So, benchmarks please, and then I'll fast-track this to commit.

On IRC it was asked "benchmark what?"

I think if we set the number of nodes on the front page to 50, used devel generate to add 50 nodes, and then added the "Recent Content" block to the sidebar, that would be a pretty decent test. Basically, we want to get a bunch of node titles on the same page, and test before/after the patch.

I very much doubt that there will be a visible change on the whole page load. Loading the front page with 50 nodes involves thousands of function calls and quite some database queries, having 50-100 method calls more is not going to make a difference. But it doesn't hurt to actually do it to have proof.

I believe we can solve the dpm() problem. If we can make the entity property API reality, then we need to make dpm() much more intelligent anyway. I think it should be possible to e.g. extract the methods a class has and list them, just like we list properties. And in the case of the Property API, we can extract the meta information about the existing properties and list them as well, as if they were real properties.

We could actually do rather crazy things with dpm(), like extracting method documentation using reflection and display it somehow or automatically create links to the api.drupal.org documentation based on class/method name.

Are we merely interested in how slower will this patch make Drupal or anybody seeing a way to access language specific versions of a property without introducing either an accessor or yet another array of doom? My understanding of what Drupal 8 is targeting has been accessors instead of more arrays of doom, and that is what the patch does. Not sure what kind of improvement do we hope for then?

Any feedback on my questions?

As mentioned in #60 I am interested in a before/after baseline so we know what it is we're dealing with performance-wise. And/or confirmation from catch that he doesn't care about this and to go ahead with commit without this information.

Anybody up to help do that?

OK, if no one wants to do it in one step, maybe we could just start by defining what

- would be considered a relevant performance test
- at which level the expected slowdown would be too much even in the light of how much something like this is required for our D8MI efforts

Once a test is agreed upon, I can probably perform it.

I think if we set the number of nodes on the front page to 50, used devel generate to add 50 nodes, and then added the "Recent Content" block to the sidebar, that would be a pretty decent test. Basically, we want to get a bunch of node titles on the same page, and test before/after the patch.

That's all we have to do

Ok, here's what I did.

- Create 100 nodes using devel generate
- Changed front page node number to 50
- Enabled recent content block

$ ab -n 100 -c 1 http://d8/

Before patch:

Requests per second:    8.74 [#/sec] (mean)
Time per request:       114.405 [ms] (mean)

Requests per second:    8.61 [#/sec] (mean)
Time per request:       116.081 [ms] (mean)

Requests per second:    8.66 [#/sec] (mean)
Time per request:       115.416 [ms] (mean)
<code>

After patch:
<code>
Requests per second:    8.73 [#/sec] (mean)
Time per request:       114.525 [ms] (mean)

Requests per second:    8.71 [#/sec] (mean)
Time per request:       114.766 [ms] (mean)

Requests per second:    8.63 [#/sec] (mean)
Time per request:       115.936 [ms] (mean)

As expected, the difference isn't measurable.

I also did some quick profiling with xdebug and the label function was called 110 times, using 0,9% of the request time. Which also explains that there is no measurable difference. What confused me a bit is that we got the expected 50 calls from node_build_content(), 10 from theme_node_recent_content() but also 50 from node_page_title(), through the menu system, not exactly sure why.

I think that's enough to set this back to RTBC, patch still applies. Will also trigger a re-test.

#54: 1616962-54.patch queued for re-testing.

I think in that test we are essentially testing the page cache after the first page hit, aren't we ? Testing with a logged-in user would possibly be more significant. I think ab can send a cookie, so logging in, catching the current session cookie and passing it to ab should enable authenticated measurement.

Perfect, thanks a lot Berdir! I'm comfortable committing this now that I know catch won't string me alive (hopefully :)).

Committed and pushed to 8.x. This will need a change notice.

Great work, guys!

More test results (100 nodes, recent content block, ab -c 1 - 100):

Anonymous users

Pre-patch:

Requests per second: 99.81 [#/sec] (mean)
Time per request: 10.019 [ms] (mean)
Requests per second: 71.32 [#/sec] (mean)
Time per request: 14.022 [ms] (mean)
Requests per second: 101.00 [#/sec] (mean)
Time per request: 9.901 [ms] (mean)

Post-patch:

Requests per second: 98.34 [#/sec] (mean)
Time per request: 10.169 [ms] (mean)
Requests per second: 96.08 [#/sec] (mean)
Time per request: 10.408 [ms] (mean)
Requests per second: 74.05 [#/sec] (mean)
Time per request: 13.505 [ms] (mean)

Logged-in users:

Pre-patch;

Requests per second: 3.71 [#/sec] (mean)
Time per request: 269.721 [ms] (mean)
Requests per second: 3.70 [#/sec] (mean)
Time per request: 270.208 [ms] (mean)
Requests per second: 3.67 [#/sec] (mean)
Time per request: 272.115 [ms] (mean)

Post-patch;

Requests per second: 3.75 [#/sec] (mean)
Time per request: 266.838 [ms] (mean)
Requests per second: 3.66 [#/sec] (mean)
Time per request: 273.501 [ms] (mean)
Requests per second: 3.62 [#/sec] (mean)
Time per request: 276.109 [ms] (mean)

So the patch does indeed not seem to have a measurable impact, even for logged-in users: deviation between test series is much higher than between series.

I added http://drupal.org/node/1697182

From a simple review of the code relating to node->label() vs. node->title, it appears that the performance hit on this patch will come from when someone overrides 'label callback', which currently as far as I can tell isn't done in node. Thus these performance benchmarks don't really apply to the real world use.

Wouldn't it be wise, in a benchmark to test performance impact of this, would be to override 'label callback' with simple O(1) and see what performance impact that has. Although a simple O(1) function is best case, and overrides will only get worse from there.

I'm not 100% sure how testing works in Drupal, but I would hope that tests written in core, could cascade down into contrib modules. Thus if a contrib module would write a slow 'label callback' perhaps a test we write now could catch the performance problem and warn people who will be using / writing contrib modules to keep these calls performant.

Just my two cents.

How would overriding the title callback with a slow function be worsened by this patch?

@j0rd: the motivation of the patch was not however to make title callback more meaningful to override, although that is a side effect. The intention was to pepare the code for multilingual properties. For an example, see #1658712: Refactor test_entity schema to be multilingual. Those overriding the title callback on their custom entities will have a performance impact, but that is not applicable to core.

Updated change record at http://drupal.org/node/1697182 with a note about input forms also using property, not label(): I got bit several times by that one when writing the patch.

fgm: Could you maybe provide a code example to go alongside your sentence? The sentence alone isn't clear to someone like me who was not involved in rolling the patch and doesn't understand the "gotchas" you ran into.

Also, let's get going again on #1616972: Replace $term->name with $term->label() too :)

Added an example for the form.

@Berdir: exactly what I meant !

Looks like change notice is good :)

Wow, this is a bit confusing. Why are entity titles called labels in the first place?

@psynaptic: There was never such a thing as an "entity title", only "node title". Label is a more generic term and the entity_label() function already existed in 7.x. This does not introduce any new concepts, it just consistently uses one that already exists for quite a long time now.

An entity label can be a node title, a comment subject, a user name, ...

Thanks for the explanation Berdir.

I ended up here via #1591806: Change block "subject" so that it's called a (admin_)label like everything else on the theme layer in which we tried to change $block->subject to $block->title since that is more consistent from a frontend perpective. I will continue this discussion in http://drupal.org/node/1591806#comment-6623190