Part of #2010184: [meta] convert ‘uri_callback’ entities param to EntityInterface::uri() method

Needs to implement Plugin\Entity\User.php uri() method and drop user_uri() function replacing it's calls to $account->uri()

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

markie’s picture

Assigned: Unassigned » markie

So I am taking a stab at this but I want to clarify a few things. Right now I have the following:

diff --git a/core/modules/user/lib/Drupal/user/Plugin/Core/Entity/User.php b/core/modules/user/lib/Drupal/user/Plugin/Core/Entity/User.php
index 6797848..beed8eb 100644
--- a/core/modules/user/lib/Drupal/user/Plugin/Core/Entity/User.php
+++ b/core/modules/user/lib/Drupal/user/Plugin/Core/Entity/User.php
@@ -179,4 +179,19 @@ class User extends Entity implements UserInterface {
   public function id() {
     return $this->uid;
   }
+
+  /**
+   * Implements Drupal\Core\Entity\EntityInterface::uid().
+   * @return array
+   */
+  public function uri() {
+    $id = $this->uid;
+    if(!$id) {
+      $id = '';
+    }
+    return array(
+      'path' => 'user/' . $id,
+      'options' => array(),
+    );
+  }
 }

I put this in Entity/User.php because I found the id() function in here.

My first question is, shouldn't both of these functions be in the UserInterface class? (Drupal\user\UserInteface.php) or more to the point, why not?

andypost’s picture

both id() and uri() are declared inEntityInterface

Once you drop

user_uri()

drop it from annotation

kgoel’s picture

Status: Active » Needs review
FileSize
1.38 KB
joachim’s picture

Status: Needs review » Needs work

Patch doesn't apply for me -- can't find the file.

herom’s picture

Status: Needs work » Needs review
FileSize
1.34 KB

Patch rerolled.

tstoeckler’s picture

Status: Needs review » Reviewed & tested by the community

Looks good.

Berdir’s picture

Status: Reviewed & tested by the community » Needs review

This should probably wait for #2083615: Use links annotation for config entity URI like for content entities and then just drop it, because it will no longer be used after that landed, I think :)

tstoeckler’s picture

I only briefly skimmed that patch, but doesn't that cover config entities only?

Berdir’s picture

Status: Needs review » Needs work

True, it does, but only because content entities already implement it. However, the canonical URL apparently still calls out to the callback and doesn't use the annotation.

That makes what I said incorrect, but it's still wrong to override uri() like this and ignore the $rel argument. Not sure what the plan is with that, exactly...

tstoeckler’s picture

... and ignore the $rel argument ...

Ahh, yes you're right. That should not be done. I had overlooked that fact. Thanks for the explanation!

tstoeckler’s picture

Issue summary: View changes

Updated issue summary.

jmarkel’s picture

Assigned: markie » jmarkel
Issue summary: View changes
jmarkel’s picture

As far as I can see, EntityInterface::uri() no longer exists.

Trying to track down where it went and why, and see if this issue is relevant anymore.

Berdir’s picture

Assigned: jmarkel » Unassigned

See #2010132: Canonical taxonomy term link for forum vocabulary is broken, try to just remove the definition and callback method, it's apparently not used anymore.

andypost’s picture

lokapujya’s picture

Status: Needs work » Needs review
FileSize
963 bytes
Berdir’s picture

Status: Needs review » Reviewed & tested by the community

Nice.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs change record

We need a change record. user_uri was a D7 function https://api.drupal.org/api/drupal/modules%21user%21user.module/function/...

alexpott’s picture

I guess all the *_uri removals should share a change record.

alexpott’s picture

Status: Needs work » Reviewed & tested by the community
Issue tags: -Needs change record

Discussed with @Berdir in IRC - "that was never an API, it was just an internal callback, do we really need a change record for that?" , "we will need a change record for removing the uri_callback feature, but I think we can do that in the parent issue when they're all gone?"

I agree.

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed aa5bf1b and pushed to 8.x. Thanks!

  • alexpott committed aa5bf1b on 8.x
    Issue #2008616 by lokapujya, herom, kgoel | andypost: Convert user_uri...

Status: Fixed » Closed (fixed)

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