Updated: Comment 0

Problem/Motivation

Most access checkers need the current account, as access is most of the time bound to the actual user.

Proposed resolution

Add an $account parameters to the access method, so the code would look like this:

Before

  public function access(Route $route, Request $request) {
    $account = $request->attributes->get('_account');
    $permission = $route->getRequirement('_permission');
    // @todo Replace user_access() with a correctly injected and session-using
    //   alternative.
    // If user_access() fails, return NULL to give other checks a chance.
    return $account->hasPermission($permission) ? static::ALLOW : static::DENY;
  }

After

  public function access(Route $route, Request $request, AccountInterface $account) {
    $permission = $route->getRequirement('_permission');
    // @todo Replace user_access() with a correctly injected and session-using
    //   alternative.
    // If user_access() fails, return NULL to give other checks a chance.
    return $account->hasPermission($permission) ? static::ALLOW : static::DENY;
  }

Remaining tasks

User interface changes

API changes

Original report by @fubhy

This makes it much easier for our route access checks to properly use the currently logged in account. Now that we have it on the request we can easily forward it to our access checks.

Files: 
CommentFileSizeAuthor
#91 access-2048223-91.patch90.21 KBtim.plunkett
PASSED: [[SimpleTest]]: [MySQL] 60,083 pass(es). View
#89 access-2048223-89.patch90.75 KBdawehner
PASSED: [[SimpleTest]]: [MySQL] 59,249 pass(es). View
#89 interdiff.txt680 bytesdawehner
#88 access-2048223-88.patch91.41 KBdawehner
PASSED: [[SimpleTest]]: [MySQL] 59,785 pass(es). View
#88 interdiff.txt7.28 KBdawehner
#84 access-2048223-84.patch91.12 KBherom
PASSED: [[SimpleTest]]: [MySQL] 59,489 pass(es). View
#84 interdiff-2048223-82-84.txt4.44 KBherom
#82 access-2048223-82.patch89.32 KBdawehner
FAILED: [[SimpleTest]]: [MySQL] 59,188 pass(es), 0 fail(s), and 280 exception(s). View
#82 interdiff.txt10.55 KBdawehner
#80 2048223-80-account-accesscheckinterface.patch79.73 KBherom
FAILED: [[SimpleTest]]: [MySQL] 59,044 pass(es), 139 fail(s), and 287 exception(s). View
#80 interdiff-2048223-78-80.txt2.75 KBherom
#78 2048223-78-account-accesscheckinterface.patch78.04 KBherom
FAILED: [[SimpleTest]]: [MySQL] 58,408 pass(es), 457 fail(s), and 2,301 exception(s). View
#78 interdiff-2048223-75-78.txt4.11 KBherom
#75 2048223-75-account-accesscheckinterface.patch75.17 KBjoelpittet
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: failed to enable simpletest module. View
#70 interdiff.txt2.18 KBdawehner
#70 access-2048223-70.patch79.63 KBdawehner
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch access-2048223-70_0.patch. Unable to apply patch. See the log in the details link for more information. View
#61 access_manager-2048223-61.patch77.45 KBParisLiakos
FAILED: [[SimpleTest]]: [MySQL] 58,782 pass(es), 1 fail(s), and 0 exception(s). View
#61 interdiff.txt7.89 KBParisLiakos
#59 access_manager-2048223-59.patch74.58 KBParisLiakos
FAILED: [[SimpleTest]]: [MySQL] 58,788 pass(es), 3 fail(s), and 0 exception(s). View
#59 interdiff.txt3.23 KBParisLiakos
#57 access_manager-2048223-57.patch72.36 KBParisLiakos
FAILED: [[SimpleTest]]: [MySQL] 58,531 pass(es), 4 fail(s), and 0 exception(s). View
#57 interdiff.txt4.96 KBParisLiakos
#55 access_manager-2048223-55.patch68 KBParisLiakos
FAILED: [[SimpleTest]]: [MySQL] 59,002 pass(es), 90 fail(s), and 356 exception(s). View
#55 interdiff.txt1.12 KBParisLiakos
#53 access_manager-2048223-53.patch67.36 KBParisLiakos
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed. View
#53 interdiff.txt13.96 KBParisLiakos
#51 access_manager-2048223-51.patch53.4 KBParisLiakos
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed. View
#49 access_manager-2048223-49.patch54.55 KBParisLiakos
FAILED: [[SimpleTest]]: [MySQL] 59,347 pass(es), 6 fail(s), and 0 exception(s). View
#43 access_manager-2048223-43.patch55.49 KBdawehner
FAILED: [[SimpleTest]]: [MySQL] 58,066 pass(es), 3 fail(s), and 0 exception(s). View
#43 interdiff.txt2.18 KBdawehner
#41 access_manager-2048223-41.patch55.16 KBdawehner
FAILED: [[SimpleTest]]: [MySQL] 58,080 pass(es), 2 fail(s), and 1 exception(s). View
#38 access-2048223-38.patch57.5 KBdawehner
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch access-2048223-38.patch. Unable to apply patch. See the log in the details link for more information. View
#38 interdiff.txt3.2 KBdawehner
#36 access-2048223-36.patch54.82 KBdawehner
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: failed to login to test site. View
#34 access-2048223-34.patch54.81 KBdawehner
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch access-2048223-34.patch. Unable to apply patch. See the log in the details link for more information. View
#32 access-2048223-32.patch51.92 KBdawehner
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch access-2048223-32.patch. Unable to apply patch. See the log in the details link for more information. View
#32 interdiff.txt15.18 KBdawehner
#30 access_manager-2048223-30.patch42.86 KBdawehner
FAILED: [[SimpleTest]]: [MySQL] 55,393 pass(es), 122 fail(s), and 24 exception(s). View
#30 interdiff.txt1.65 KBdawehner
#28 access-account-2048223-28.patch42.7 KBdawehner
FAILED: [[SimpleTest]]: [MySQL] 57,703 pass(es), 72 fail(s), and 12 exception(s). View
#28 interdiff.txt620 bytesdawehner
#26 access-2048223-26.patch42.68 KBdawehner
FAILED: [[SimpleTest]]: [MySQL] 6,996 pass(es), 940 fail(s), and 148 exception(s). View
#26 interdiff.txt566 bytesdawehner
#23 access-2048223-23.patch42.4 KBdawehner
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed. View
#23 interdiff.txt31.34 KBdawehner
#20 2048223-access-20.patch32.01 KBvijaycs85
FAILED: [[SimpleTest]]: [MySQL] 3,217 pass(es), 1,255 fail(s), and 88 exception(s). View
#20 2048223-diff-18-20.txt594 bytesvijaycs85
#18 2048223-access-18.patch31.43 KBvijaycs85
FAILED: [[SimpleTest]]: [MySQL] 57,654 pass(es), 343 fail(s), and 1,049 exception(s). View
#18 2048223-diff-16-18.txt481 bytesvijaycs85
#16 access-2048223-16.patch31.21 KBdawehner
FAILED: [[SimpleTest]]: [MySQL] 57,287 pass(es), 342 fail(s), and 1,049 exception(s). View
#14 drupal-2048223-14.patch33.41 KBdawehner
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: failed to login to test site. View
#12 drupal-2048223-12.patch34.95 KBdawehner
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details.. View
#12 interdiff.txt580 bytesdawehner
#10 drupal-2048223-10.patch34.39 KBdawehner
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: failed to login to test site. View
#10 interdiff.txt0 bytesdawehner
#8 2048223-8.patch34.39 KBdamiankloip
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: failed to login to test site. View
#8 interdiff-2048223-8.txt1.09 KBdamiankloip
#6 2048223-6.patch33.29 KBdamiankloip
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed. View
#6 interdiff-2048223-6.txt22.3 KBdamiankloip
#3 2048223-3.patch33.24 KBfubhy
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed. View
#1 2048223-1.patch34.32 KBfubhy
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed. View

Comments

fubhy’s picture

Status: Active » Needs review
FileSize
34.32 KB
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed. View
fubhy’s picture

+++ b/core/modules/content_translation/content_translation.moduleundefined
@@ -337,7 +337,7 @@ function content_translation_edit_access(EntityInterface $entity, Language $lang
- *   The entity being translated.
+ *   The entity being translated.chx
  * @param \Drupal\Core\Language\Language $language
  *   (optional) The language of the translated values. Defaults to the current
  *   content language.
@@ -358,7 +358,7 @@ function content_translation_library_info() {

@@ -358,7 +358,7 @@ function content_translation_library_info() {
     'title' => 'Content translation UI',
     'version' => VERSION,
     'js' => array(
-      $path . '/content_translation.admin.js' => array(),
+      $path . '/conteant_translation.admin.js' => array(),
     ),
     'css' => array(

Hahaha, I guess I was trying to write on IRC when I wrote that patch :D

fubhy’s picture

FileSize
33.24 KB
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed. View

Re-uploading without the reference to chx :)

Status: Needs review » Needs work

The last submitted patch, 2048223-3.patch, failed testing.

Crell’s picture

Let's put the account parameter as the 3rd argument for the method, and make it optional (default NULL). I think that will make this change backward-compatible. (I agree it's a good chance, but we need to minimize breakage.)

damiankloip’s picture

Status: Needs work » Needs review
FileSize
22.3 KB
33.29 KB
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed. View

I think I like that idea better, let's move that param.

Status: Needs review » Needs work

The last submitted patch, 2048223-6.patch, failed testing.

damiankloip’s picture

Status: Needs work » Needs review
FileSize
1.09 KB
34.39 KB
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: failed to login to test site. View

Status: Needs review » Needs work

The last submitted patch, 2048223-8.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
FileSize
0 bytes
34.39 KB
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: failed to login to test site. View

You just missed some places.

Status: Needs review » Needs work

The last submitted patch, drupal-2048223-10.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
FileSize
580 bytes
34.95 KB
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details.. View

For now we need this fix.

Status: Needs review » Needs work

The last submitted patch, drupal-2048223-12.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
FileSize
33.41 KB
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: failed to login to test site. View

I explicitly removed the empty AccessInterface from the interface, so it is not an api change.

Status: Needs review » Needs work

The last submitted patch, drupal-2048223-14.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
FileSize
31.21 KB
FAILED: [[SimpleTest]]: [MySQL] 57,287 pass(es), 342 fail(s), and 1,049 exception(s). View

Rerolled.

Status: Needs review » Needs work

The last submitted patch, access-2048223-16.patch, failed testing.

vijaycs85’s picture

Status: Needs work » Needs review
FileSize
481 bytes
31.43 KB
FAILED: [[SimpleTest]]: [MySQL] 57,654 pass(es), 343 fail(s), and 1,049 exception(s). View

Re-roll + updating interface with $account.

Status: Needs review » Needs work

The last submitted patch, 2048223-access-18.patch, failed testing.

vijaycs85’s picture

Status: Needs work » Needs review
FileSize
594 bytes
32.01 KB
FAILED: [[SimpleTest]]: [MySQL] 3,217 pass(es), 1,255 fail(s), and 88 exception(s). View

seems this swap in arguments position fix most of the fails.

Status: Needs review » Needs work

The last submitted patch, 2048223-access-20.patch, failed testing.

dawehner’s picture

The problem with the patch in #20 is that this is sadly an api change.

dawehner’s picture

Title: Add $account argument to AccessCheckInterface::access() method. » Add $account argument to AccessCheckInterface::access() method and use the curernt_user service
Status: Needs work » Needs review
FileSize
31.34 KB
42.4 KB
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed. View

Let's move the issue a bit.

After #2062151: Create a current user service to ensure that current account is always available we have a way to properly inject the account into the access manager.

Status: Needs review » Needs work

The last submitted patch, access-2048223-23.patch, failed testing.

dawehner’s picture

Mh ...

Symfony\Component\DependencyInjection\Exception\ScopeWideningInjectionException: Scope Widening Injection detected: The definition "access_manager" references the service "current_user" which belongs to a narrower scope. Generally, it is safer to either move "access_manager" to scope "request" or alternatively rely on the provider pattern by injecting the container itself, and requesting the service "current_user" each time it is needed. In rare, special cases however that might not be necessary, then you can set the reference to strict=false to get rid of this error. in Symfony\Component\DependencyInjection\Compiler\CheckReferenceValidityPass->validateScope() (line 144 of /var/www/d8/core/vendor/symfony/dependency-injection/Symfony/Component/DependencyInjection/Compiler/CheckReferenceValidityPass.php).
dawehner’s picture

Status: Needs work » Needs review
FileSize
566 bytes
42.68 KB
FAILED: [[SimpleTest]]: [MySQL] 6,996 pass(es), 940 fail(s), and 148 exception(s). View

There we go.

Status: Needs review » Needs work

The last submitted patch, access-2048223-26.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
FileSize
620 bytes
42.7 KB
FAILED: [[SimpleTest]]: [MySQL] 57,703 pass(es), 72 fail(s), and 12 exception(s). View

This should fix most of the failures.

Status: Needs review » Needs work

The last submitted patch, access-account-2048223-28.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
FileSize
1.65 KB
42.86 KB
FAILED: [[SimpleTest]]: [MySQL] 55,393 pass(es), 122 fail(s), and 24 exception(s). View

Sorry for all the noise, not sure whether this is nice, but I figured out why it actually fails:

Symfony\Component\DependencyInjection\ContainerBuilder->shareService(Object, Object, 'access_subscriber')
Symfony\Component\DependencyInjection\ContainerBuilder->createService(Object, 'access_subscriber')
Symfony\Component\DependencyInjection\ContainerBuilder->get('access_subscriber')
Symfony\Component\EventDispatcher\ContainerAwareEventDispatcher->lazyLoad('routing.route_alter')
Symfony\Component\EventDispatcher\ContainerAwareEventDispatcher->dispatch('routing.route_alter', Object)
Drupal\Core\Routing\RouteBuilder->rebuild()
views_invalidate_cache()
Drupal\views\Entity\View->postSave(Object, 1)
Drupal\Core\Config\Entity\ConfigStorageController->save(Object)
Drupal\Core\Entity\Entity->save()
Drupal\block\Tests\Views\DisplayBlockTest->testDeleteBlockDisplay()

So the test code triggers are route rebuild, which gets the access subscriber, so the access manager which then relies on the current user.
Potentially splitting up the access manager or just make the current user optional might be a solution.

Status: Needs review » Needs work

The last submitted patch, access_manager-2048223-30.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
Issue tags: +API change, +WSCCI, +Stalking Crell, +Routing
FileSize
15.18 KB
51.92 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch access-2048223-32.patch. Unable to apply patch. See the log in the details link for more information. View

Splitted up the access subscriber from being dependent on the current user/request and one just handling the rebuilding.
Added the current user to the request aware event subscriber and pass it further to the access manager.

Status: Needs review » Needs work

The last submitted patch, access-2048223-32.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
FileSize
54.81 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch access-2048223-34.patch. Unable to apply patch. See the log in the details link for more information. View

Missing file.

Status: Needs review » Needs work

The last submitted patch, access-2048223-34.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
FileSize
54.82 KB
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: failed to login to test site. View

Just a rerole.

Status: Needs review » Needs work

The last submitted patch, access-2048223-36.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
FileSize
3.2 KB
57.5 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch access-2048223-38.patch. Unable to apply patch. See the log in the details link for more information. View

Let's see.

Status: Needs review » Needs work

The last submitted patch, access-2048223-38.patch, failed testing.

tim.plunkett’s picture

Title: Add $account argument to AccessCheckInterface::access() method and use the curernt_user service » Add $account argument to AccessCheckInterface::access() method and use the current_user service
dawehner’s picture

Status: Needs work » Needs review
FileSize
55.16 KB
FAILED: [[SimpleTest]]: [MySQL] 58,080 pass(es), 2 fail(s), and 1 exception(s). View

Here is a rerole.

------------
45e492dc05f38d697e9833fcef6bb53580aa7b94

Status: Needs review » Needs work

The last submitted patch, access_manager-2048223-41.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
FileSize
2.18 KB
55.49 KB
FAILED: [[SimpleTest]]: [MySQL] 58,066 pass(es), 3 fail(s), and 0 exception(s). View

Let's upload the views fix.

Status: Needs review » Needs work

The last submitted patch, access_manager-2048223-43.patch, failed testing.

tim.plunkett’s picture

In EntityAccessController:

 public function access(EntityInterface $entity, $operation, $langcode = Language::LANGCODE_DEFAULT, AccountInterface $account = NULL) {
 public function createAccess($entity_bundle = NULL, AccountInterface $account = NULL, array $context = array()) {

This becomes strange, because $account is now always passed.

Right now we have to do $account = $this->prepareUser($account); at the beginning of each, which is just

  protected function prepareUser(AccountInterface $account = NULL) {
    if (!$account) {
      $account = $GLOBALS['user'];
    }
    return $account;
  }
YesCT’s picture

this issue might provide a more generic solution to what we need to get config_translation into core. #2064697: Remove user_access from ConfigNameCheck

dawehner’s picture

GRRR.

catch’s picture

ParisLiakos’s picture

Status: Needs work » Needs review
FileSize
54.55 KB
FAILED: [[SimpleTest]]: [MySQL] 59,347 pass(es), 6 fail(s), and 0 exception(s). View

ok so TaxonomyTermCreateAccess moved to EntityCreateAccessCheck see #2068287: Support bundle names provided in the request arguments in EntityCreateAccessCheck

lets see how much i messed up the reroll

Status: Needs review » Needs work

The last submitted patch, access_manager-2048223-49.patch, failed testing.

ParisLiakos’s picture

Assigned: Unassigned » ParisLiakos
Status: Needs work » Needs review
FileSize
53.4 KB
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed. View

just a reroll
core/modules/block/lib/Drupal/block/Access/BlockThemeAccessCheck is dead thats why this is smaller

Status: Needs review » Needs work

The last submitted patch, access_manager-2048223-51.patch, failed testing.

ParisLiakos’s picture

Status: Needs work » Needs review
FileSize
13.96 KB
67.36 KB
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed. View

Of course we have new access checkers. the drop moves!

Status: Needs review » Needs work

The last submitted patch, access_manager-2048223-53.patch, failed testing.

ParisLiakos’s picture

Status: Needs work » Needs review
FileSize
1.12 KB
68 KB
FAILED: [[SimpleTest]]: [MySQL] 59,002 pass(es), 90 fail(s), and 356 exception(s). View

Status: Needs review » Needs work

The last submitted patch, access_manager-2048223-55.patch, failed testing.

ParisLiakos’s picture

Status: Needs work » Needs review
FileSize
4.96 KB
72.36 KB
FAILED: [[SimpleTest]]: [MySQL] 58,531 pass(es), 4 fail(s), and 0 exception(s). View

Status: Needs review » Needs work

The last submitted patch, access_manager-2048223-57.patch, failed testing.

ParisLiakos’s picture

Status: Needs work » Needs review
FileSize
3.23 KB
74.58 KB
FAILED: [[SimpleTest]]: [MySQL] 58,788 pass(es), 3 fail(s), and 0 exception(s). View

the HandlerFilterUserNameTest test passes locally:/

now..i believe the route enhancers we have for authentication...hmm dunno, they come pretty late obviously..
i ll check tomorrow

this should fix at least 1 test

Status: Needs review » Needs work

The last submitted patch, access_manager-2048223-59.patch, failed testing.

ParisLiakos’s picture

Status: Needs work » Needs review
FileSize
7.89 KB
77.45 KB
FAILED: [[SimpleTest]]: [MySQL] 58,782 pass(es), 1 fail(s), and 0 exception(s). View

synchronized FTW

Status: Needs review » Needs work

The last submitted patch, access_manager-2048223-61.patch, failed testing.

ParisLiakos’s picture

Assigned: ParisLiakos » Unassigned

the remaining failures, pass locally, so should be php5.3/enviroment specific errors

dawehner’s picture

wow! Just wondering why the same tests failed before but we need all the synchronized services for this issue. I totally agree that it is AWESOME that you found
all these bugs

ParisLiakos’s picture

yes i agree its awesome:D

#59 was failing in access tests
in #61 those are fixed cause of the changes in the access subscriber, but we got two new in Drupal\block\Tests\Views\DisplayBlockTest

i ll ask for a re-test in case some of them where random.

about synced: i think we should do the same for the request, but in a different issue;)

ParisLiakos’s picture

Status: Needs work » Needs review
Issue tags: -Needs issue summary update, -API change, -WSCCI, -Stalking Crell, -Approved API change, -Routing

#61: access_manager-2048223-61.patch queued for re-testing.

Status: Needs review » Needs work
Issue tags: +Needs issue summary update, +API change, +WSCCI, +Stalking Crell, +Approved API change, +Routing

The last submitted patch, access_manager-2048223-61.patch, failed testing.

dawehner’s picture

+++ b/core/core.services.yml
@@ -177,7 +177,8 @@ services:
   plugin.manager.menu.local_task:
     class: Drupal\Core\Menu\LocalTaskManager
-    arguments: ['@controller_resolver', '@request', '@router.route_provider', '@module_handler', '@cache.cache', '@language_manager', '@access_manager']
+    arguments: ['@controller_resolver', '@request', '@router.route_provider', '@module_handler', '@cache.cache', '@language_manager', '@access_manager', '@current_user']
+    scope: request

Urgs, I would really like to avoid this if possible.

ParisLiakos’s picture

it just says the truth:) i dont see whats bad about it. it depends on the request, so it is in request scope

dawehner’s picture

Status: Needs work » Needs review
FileSize
79.63 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch access-2048223-70_0.patch. Unable to apply patch. See the log in the details link for more information. View
2.18 KB

This test is pretty dump, so maybe this works ... but well

Status: Needs review » Needs work
Issue tags: -Needs issue summary update, -API change, -WSCCI, -Stalking Crell, -Approved API change, -Routing

The last submitted patch, access-2048223-70.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review

#70: access-2048223-70.patch queued for re-testing.

Status: Needs review » Needs work
Issue tags: +Needs issue summary update, +API change, +WSCCI, +Stalking Crell, +Approved API change, +Routing

The last submitted patch, access-2048223-70.patch, failed testing.

dawehner’s picture

This is blocked on doing something sane for #2095125: Use access constants in every access control context

joelpittet’s picture

Status: Needs work » Needs review
Issue tags: -WSCCI, -Stalking Crell, -Routing
FileSize
75.17 KB
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: failed to enable simpletest module. View

Re-rolled #70

joelpittet’s picture

Issue tags: +WSCCI, +Stalking Crell, +Routing

adding tags back, wtf

Status: Needs review » Needs work

The last submitted patch, 2048223-75-account-accesscheckinterface.patch, failed testing.

herom’s picture

Status: Needs work » Needs review
FileSize
4.11 KB
78.04 KB
FAILED: [[SimpleTest]]: [MySQL] 58,408 pass(es), 457 fail(s), and 2,301 exception(s). View

get #75 past the install fails. also fix some Drupal → \Drupal cases.

Status: Needs review » Needs work

The last submitted patch, 2048223-78-account-accesscheckinterface.patch, failed testing.

herom’s picture

Assigned: Unassigned » herom
Status: Needs work » Needs review
FileSize
2.75 KB
79.73 KB
FAILED: [[SimpleTest]]: [MySQL] 59,044 pass(es), 139 fail(s), and 287 exception(s). View

patch update.

Status: Needs review » Needs work

The last submitted patch, 2048223-80-account-accesscheckinterface.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
FileSize
10.55 KB
89.32 KB
FAILED: [[SimpleTest]]: [MySQL] 59,188 pass(es), 0 fail(s), and 280 exception(s). View

Somes fixes here and there.

Status: Needs review » Needs work

The last submitted patch, access-2048223-82.patch, failed testing.

herom’s picture

Assigned: herom » Unassigned
Status: Needs work » Needs review
FileSize
4.44 KB
91.12 KB
PASSED: [[SimpleTest]]: [MySQL] 59,489 pass(es). View

another update. hoping for green.

ParisLiakos’s picture

hey its green!

  1. +++ b/core/modules/aggregator/lib/Drupal/aggregator/Access/CategoriesAccessCheck.php
    @@ -44,10 +45,10 @@ public function appliesTo() {
         // @todo Replace user_access() with a correctly injected and session-using
         // alternative.
    

    lets remove this comment too

  2. +++ b/core/modules/user/lib/Drupal/user/Tests/Views/HandlerFilterUserNameTest.php
    @@ -84,20 +84,17 @@ protected function setUp() {
    -    // Test all of the accounts with a single entry.
    ...
    -    foreach ($this->accounts as $account) {
    -      $view->filter['uid']->value = array($account->id());
    -    }
    +    $view->filter['uid']->value = array($this->accounts[0]->id());
    ...
    -    $this->assertIdenticalResultset($view, array(array('uid' => $account->id())), $this->columnMap);
    +    $this->assertIdenticalResultset($view, array(array('uid' => $this->accounts[0]->id())), $this->columnMap);
    ...
    -  public function testAdminUserInterface() {
    +  public function ptestAdminUserInterface() {
    
    @@ -140,7 +137,7 @@ public function testAdminUserInterface() {
    -  public function testExposedFilter() {
    +  public function ptestExposedFilter() {
    
    +++ b/core/modules/views/lib/Drupal/views/Plugin/views/filter/InOperator.php
    @@ -384,6 +384,7 @@ protected function opSimple() {
    +    debug($this->value);
    

    So lets revert those chnages now..unless the test ones are actually needed?

  3. +++ b/core/tests/Drupal/Tests/Core/Access/DefaultAccessCheckTest.php
    @@ -40,6 +47,7 @@ public static function getInfo() {
    +    $this->account = $this->getMock('Drupal\Core\Session\AccountInterface');
    
    +++ b/core/tests/Drupal/Tests/Core/Entity/EntityAccessCheckTest.php
    @@ -50,7 +50,8 @@ public function testAccess() {
    +    $account = $this->getMock('Drupal\Core\Session\AccountInterface');
    
    +++ b/core/tests/Drupal/Tests/Core/Entity/EntityCreateAccessCheckTest.php
    @@ -118,7 +118,8 @@ public function testAccess($entity_bundle, $requirement, $access, $expected) {
    +    $account = $this->getMock('Drupal\Core\Session\AccountInterface');
    
    +++ b/core/tests/Drupal/Tests/UnitTestCase.php
    @@ -181,4 +181,13 @@ public function getStringTranslationStub() {
    +  public function getAccountStub() {
    

    So i see we got a getAccountStub method, although its more of a mock:P
    I am not sure it makes much sense to have a method here?
    But anyway we should update those calls, to call the method instead

tim.plunkett’s picture

I don't see how getAccountStub is useful. It's a one liner, just mock the interface you want.

dawehner’s picture

I don't see how getAccountStub is useful. It's a one liner, just mock the interface you want.

Well, the reason I added it was because this is potentially needed in a hell lot of places.

dawehner’s picture

FileSize
7.28 KB
91.41 KB
PASSED: [[SimpleTest]]: [MySQL] 59,785 pass(es). View

So lets revert those changes now..unless the test ones are actually needed?

It is needed to fix the failures. If you have a look at the existing code you really wonder how this could have ever passed.
It did not worked on purpose and got broken somehow by that patch.

Fixed the other stuff.

dawehner’s picture

FileSize
680 bytes
90.75 KB
PASSED: [[SimpleTest]]: [MySQL] 59,249 pass(es). View

No debug().

tim.plunkett’s picture

Status: Needs review » Reviewed & tested by the community
+++ b/core/core.services.yml
@@ -631,6 +642,7 @@ services:
+    synchronized: true

+++ b/core/lib/Drupal/Core/DependencyInjection/YamlFileLoader.php
@@ -107,6 +107,10 @@ protected function parseDefinition($id, $service, $filename) {
+    if (isset($service['synchronized'])) {
+      $definition->setSynchronized($service['synchronized']);
+    }

Nice!

This looks ready to go.

tim.plunkett’s picture

FileSize
90.21 KB
PASSED: [[SimpleTest]]: [MySQL] 60,083 pass(es). View

_search_menu_access() was removed, so this needed a reroll.

catch’s picture

+++ b/core/core.services.yml
@@ -192,10 +192,11 @@ services:
+    scope: request

Why is the request scope added here, when we had to remove it from the current user service in #2076411: Remove the request scope from the current user service? We have a critical to add that back, would just like to know why it breaks without it or whether this is a nicety.

Otherwise looks great.

ParisLiakos’s picture

i added it before #2076411: Remove the request scope from the current user service went in. now its not needed, but we wiill have to add it back when current_user get under request scope again. so i think just leaving it there its ok

catch’s picture

Issue tags: +Avoid commit conflicts

Hmm OK. We have that critical to add it back, so it'll all make sense again once that happens.

Patch doesn't apply any more unfortunately.

catch’s picture

#91: access-2048223-91.patch queued for re-testing.

joelpittet’s picture

Patch seemed to apply here still.

catch’s picture

Title: Add $account argument to AccessCheckInterface::access() method and use the current_user service » Change notice: Add $account argument to AccessCheckInterface::access() method and use the current_user service
Priority: Critical » Major
Status: Reviewed & tested by the community » Active

Hmm yes my mistake locally..

Committed/pushed to 8.x, thanks! Will need a change notice.

dawehner’s picture

dawehner’s picture

Issue summary: View changes

todo

effulgentsia’s picture

Issue summary: View changes
Issue tags: -Avoid commit conflicts
xjm’s picture

Status: Active » Needs review
Issue tags: +Needs change record

@dawehner, I guess this is "Needs review" for the change record updates you list? Do we need to add anything else?

dawehner’s picture

Status: Needs review » Fixed

I think we already cover now all in the existing change notifications.

Berdir’s picture

Title: Change notice: Add $account argument to AccessCheckInterface::access() method and use the current_user service » Add $account argument to AccessCheckInterface::access() method and use the current_user service
Issue tags: -Needs change record

Status: Fixed » Closed (fixed)

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