Support from Acquia helps fund testing for Drupal Acquia logo

Comments

larowlan’s picture

Assigned: Unassigned » larowlan

I'll tackle this one.

larowlan’s picture

Status: Active » Needs review
FileSize
104.53 KB

First crack

Status: Needs review » Needs work

The last submitted patch, user-access-1862752.2.patch, failed testing.

larowlan’s picture

Status: Needs work » Needs review

#2: user-access-1862752.2.patch queued for re-testing.

Status: Needs review » Needs work

The last submitted patch, user-access-1862752.2.patch, failed testing.

larowlan’s picture

Status: Needs work » Needs review
FileSize
5.12 KB

needed a merge against 8.x

Status: Needs review » Needs work

The last submitted patch, user-access-1862752.6.patch, failed testing.

larowlan’s picture

Status: Needs work » Needs review
FileSize
392 bytes
5.12 KB

credit for jthorson for setting me straight on this

Status: Needs review » Needs work

The last submitted patch, user-access-1862752.8.patch, failed testing.

larowlan’s picture

these two tests pass locally, kicking off again.

larowlan’s picture

Status: Needs work » Needs review

#8: user-access-1862752.8.patch queued for re-testing.

Berdir’s picture

+++ b/core/modules/user/lib/Drupal/user/UserAccessController.phpundefined
@@ -0,0 +1,74 @@
+      if ($account->uid == $uid || user_access('administer users')) {
+        return TRUE;
+      }
+      elseif (user_access('access user profiles')) {
+        // Only allow view access if the account is active.
+        return $entity->status;

$account should be passed to these user_access() calls. Same for a few more below. Some do it correctly.

larowlan’s picture

Fixes issues identified by @Berdir and also an incorrect use of $account->uid > 0 which should have been $entity->uid.

Status: Needs review » Needs work

The last submitted patch, user-access-1862752.13.patch, failed testing.

larowlan’s picture

Status: Needs work » Needs review

#13: user-access-1862752.13.patch queued for re-testing.

Status: Needs review » Needs work

The last submitted patch, user-access-1862752.13.patch, failed testing.

Berdir’s picture

+++ b/core/modules/user/lib/Drupal/user/Plugin/Core/Entity/User.phpundefined
@@ -19,6 +19,8 @@
+ *   render_controller_class = "Drupal\user\UserRenderController",
+ *   access_controller_class = "Drupal\user\UserAccessController",

There is no custom render controller for users anymore, I guess that's why this failed.

larowlan’s picture

Status: Needs work » Needs review
FileSize
5.13 KB
669 bytes

@Berdir, thanks you're a legend!

Status: Needs review » Needs work

The last submitted patch, user-access-1862752.18.patch, failed testing.

Berdir’s picture

entity_test_mul: Two entities loaded by uid without caring about property translatability.	Other	EntityTranslationTest.php	196	Drupal\system\Tests\Entity\EntityTranslationTest->testMultilingualProperties()

Looks like a random test failure. I've seen a few of those already related to translation tests. Something strange is going on there...

Berdir’s picture

Status: Needs work » Needs review

#18: user-access-1862752.18.patch queued for re-testing.

Berdir’s picture

+++ b/core/modules/user/lib/Drupal/user/UserAccessController.phpundefined
@@ -0,0 +1,74 @@
+ * Contains Drupal\user\UserAccessController.

Should be \Drupal\...

+++ b/core/modules/user/user.moduleundefined
@@ -840,30 +840,20 @@ function user_register_access() {
 function user_view_access($account) {
-  $uid = is_object($account) ? $account->uid : (int) $account;
-
-  // Never allow access to view the anonymous user account.
-  if ($uid) {
-    // Admins can view all, users can view own profiles at all times.
-    if ($GLOBALS['user']->uid == $uid || user_access('administer users')) {
-      return TRUE;
-    }
-    elseif (user_access('access user profiles')) {
-      // At this point, load the complete account object.
-      if (!is_object($account)) {
-        $account = user_load($uid);
-      }
-      return (is_object($account) && $account->status);
-    }
+  if (is_numeric($account)) {
+    $account = user_load($account);
   }
-  return FALSE;
+  return $account->access('view');

I haven't found a call in core that was still calling that with a user id.

I think we can safely drop that "feature" and type hint $account here accordingly.

Same for the other functions below.

And I would suggest to only keep using this functions for access callbacks and change direct calls to ->access('view').

larowlan’s picture

Thanks, attached fixes issues at #22 as well as fixing some doc blocks now that $account is type-hinted.

Status: Needs review » Needs work

The last submitted patch, user-access-1862752.23.patch, failed testing.

larowlan’s picture

Status: Needs work » Needs review
FileSize
383 bytes
8.09 KB

meh two use Drupal\user\Plugin\Core\Entity\User;

Berdir’s picture

+++ b/core/modules/user/lib/Drupal/user/Plugin/Core/Entity/User.phpundefined
@@ -19,6 +19,8 @@
+ *   render_controller_class = "Drupal\user\UserRenderController",
+ *   access_controller_class = "Drupal\user\UserAccessController",

Magic re-appearance of the render controller class? :)

+++ b/core/modules/user/user.moduleundefined
@@ -2679,7 +2678,7 @@ function user_rdf_mapping() {
 function user_file_download_access($field, EntityInterface $entity, File $file) {
   if ($entity->entityType() == 'user') {
-    return user_view_access($entity);
+    return $entity->access('view');
   }

We can replace this with a entity-generic implementation in system or entity module as soon as all entities are converted. We might even be able to kill this hook completely and just keep the alter. I've already started with a patch for that in the issue about taxonomy terms not implementing this.

It's a nice example of what the unified access API allows us to do :)

Status: Needs review » Needs work

The last submitted patch, user-access-1862752.25.patch, failed testing.

larowlan’s picture

Status: Needs work » Needs review
FileSize
8.02 KB
669 bytes

Sun morning fail.
Definitely committed to my local branch now.

Berdir’s picture

Status: Needs review » Reviewed & tested by the community

Ok, I think this is ready to go. It has the same problems with User vs. anoymous user vs. $GLOBALS['user'] which are all different things but it's an existing problem and not blocking us here.

larowlan’s picture

Issue tags: +Needs change record

This will need a change notice as the new preferred approach is eg $entity->access('view') over user_view_access() except for access callbacks

Berdir’s picture

I guess we can update the existing access change notice, add all the conversion issues and a list of removed/deprecated functions. Speaking of that...

+++ b/core/modules/user/user.moduleundefined
@@ -837,34 +837,27 @@ function user_register_access() {
+function user_view_access(User $account) {
+  return $account->access('view');

Wondering if we want to introduce a entity_page_access(Entity $entity, $operation = 'view') to unify those three access callbacks here.

larowlan’s picture

Makes sense to me. Want a follow up?

Berdir’s picture

Either that or update the patch here, I'll try to RTBC either asap :)

larowlan’s picture

Status: Reviewed & tested by the community » Needs work

Where abouts?
entity.module?

Berdir’s picture

Let's add it to entity.inc for now, where everything else is. We haven't yet decided what exactly should happen with entity.module vs. entity.inc. Right now, the module is just the owner for the display entity type.

larowlan’s picture

Status: Needs work » Needs review
FileSize
9.6 KB
3.8 KB

Let's see how this goes.

larowlan’s picture

Def needs a change notice now we've dropped user_view_access, user_edit_access and user_cancel_access

Berdir’s picture

Yes, let's update http://drupal.org/node/1862420 with entity_page_access() (make sure to state that it should only be used for access callbacks) and the list of removed functions (including the operation that should be used for them now)

Status: Needs review » Needs work

The last submitted patch, user-access-1862752.36.patch, failed testing.

larowlan’s picture

Status: Needs work » Needs review
FileSize
9.6 KB
0 bytes

ha, 'edit' != 'update'

larowlan’s picture

Wrong interdiff

Status: Needs review » Needs work

The last submitted patch, user-access-1862752.40.patch, failed testing.

larowlan’s picture

hmm these fails look valid, menu data cached that references the old access callbacks for the upgrade tests and open id issues with same removed functions

larowlan’s picture

Status: Needs work » Needs review
FileSize
10.6 KB
1 KB

Missed some refs in openid.module

Berdir’s picture

Status: Needs review » Reviewed & tested by the community

This is good to go if it passes the tests.

Dries’s picture

+++ b/core/modules/tracker/tracker.moduleundefined
@@ -185,7 +185,7 @@ function _tracker_myrecent_access($account) {
 function _tracker_user_access($account) {
-  return user_view_access($account) && user_access('access content');
+  return $account->access('view') && user_access('access content');
 }

I think this should be 'hasAccess()' instead of 'access()'.

+++ b/core/modules/user/lib/Drupal/user/UserAccessController.phpundefined
@@ -0,0 +1,74 @@
+  public function deleteAccess(EntityInterface $entity, $langcode = LANGUAGE_DEFAULT, User $account = NULL) {

I think it would be better to name this 'hasDeleteAccess()' instead of 'deleteAccess()'. For most people, 'deleteAccess()' means we'll delete access.

I understand none of these two things are introduced in this patch, but I wanted to share my thoughts nonetheless. Maybe we can create another issue for this, if not already? Small changes like this can avoid many WTFs. :-)

Dries’s picture

Status: Reviewed & tested by the community » Fixed

Other than these API suggestions, I think this looks good. Committed to 8.x. Thanks.

Dave Reid’s picture

hasAccess() sounds weird, like we're checking if the entity itself has access to something. Then again I can't really think of a better alternative besides leaving it 'access'.

Status: Fixed » Closed (fixed)

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

xjm’s picture

Priority: Normal » Major
Status: Closed (fixed) » Active

This was marked for a change notice update but I don't see the change notice referenced in the summary. Can we make sure the change notice got updated and add this issue to its issue list? Thanks!

Berdir’s picture

Priority: Major » Normal
Status: Active » Fixed
Issue tags: -Needs change record

Added this to https://drupal.org/node/1862420, but this was just a conversion issue, none of the others were added either.

larowlan’s picture

Assigned: larowlan » Unassigned

Status: Fixed » Closed (fixed)

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