Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Call to method Drupal\Core\Entity\Query\QueryInterface::accessCheck() with false will always evaluate to true. #508

Closed
acbramley opened this issue Feb 2, 2023 · 8 comments · Fixed by #512

Comments

@acbramley
Copy link

We just updated to 1.1.28 and every single accessCheck(FALSE) call is throwing this error.

@mglaman
Copy link
Owner

mglaman commented Feb 2, 2023

This is an unexpected side effect from the type-specifying extension added in #497.

Can you paste a sample of the code? I feel the test suite didn't fail due to tests not running with only a specific set of rules. I am assuming it is chained methods causing this error.

@acbramley
Copy link
Author

Sure @mglaman sorry I should've included some examples

    $ids = $entityStorage
      ->getQuery()
      ->currentRevision()
      ->accessCheck(FALSE)
      ->condition($idField, $entityId)
      ->execute();
    $ids = \Drupal::entityQuery('foo')
      ->accessCheck(TRUE)
      ->condition('status', 1)
      ->condition('type', 'bar')
      ->condition('baz', $this->id())
      ->execute();
    $query = $storage->getQuery()
      ->accessCheck(FALSE)
      ->condition($bundleKey, $bundle);
    $query = $storage->getQuery()
      ->accessCheck(FALSE)
      ->sort('id', 'DESC')
      ->range(0, 1);

Even if I split the accessCheck onto its own line I get the same issue. E.g

    $storage = \Drupal::entityTypeManager()->getStorage($entityTypeRepository->getEntityTypeFromClass(static::class));
    $query = $storage->getQuery();
    if ($bundles) {
      $query->condition($storage->getEntityType()->getKey('bundle'), $bundles, 'IN');
    }
    return (int) $query->count()->accessCheck(FALSE)->execute();

and

    $storage = \Drupal::entityTypeManager()->getStorage($entityTypeRepository->getEntityTypeFromClass(static::class));
    $query = $storage->getQuery();
    if ($bundles) {
      $query->condition($storage->getEntityType()->getKey('bundle'), $bundles, 'IN');
    }
    $query->count();
    $query->accessCheck(FALSE);
    return (int) $query->execute();

both error.

@acbramley
Copy link
Author

Here's a trimmed down paste of our phpstan config too if that changes anything

parameters:
	level: 6
	featureToggles:
		curlSetOptTypes: true
		listType: true
		missingMagicSerializationRule: true
		nullContextForVoidReturningFunctions: true
		unescapeStrings: true
	paths:
		- console
		- src
		- tests/Unit
		- app/modules/custom
		- app/profiles/
		- app/themes
	ignoreErrors:
		- '#extends @internal class#'
		- '#Call to deprecated method prophesize#'
		- '#Access to an undefined property Drupal\\Core\\Entity\\EntityInterface::\$original\.#'
	strictRules:
		disallowedLooseComparison: false
		booleansInConditions: false
		uselessCast: false
		requireParentConstructorCall: false
		disallowedConstructs: false
		overwriteVariablesWithLoop: false
		closureUsesThis: false
		matchingInheritedMethodNames: false
		numericOperandsInArithmeticOperators: false
		strictCalls: false
		switchConditionsMatchingType: false
		noVariableVariables: false
	reportUnmatchedIgnoredErrors: true
	checkMissingIterableValueType: false
	checkGenericClassInNonGenericObjectType: false
	treatPhpDocTypesAsCertain: false
	fileExtensions:
		- php
		- module
		- theme
		- profile
		- install
		- inc

@acbramley
Copy link
Author

Thanks to @dpi for pointing out the errors go away when strict is disabled by removing the phpstan/phpstan-strict-rules package and commenting out the strictRules config.

@ptmkenny
Copy link

ptmkenny commented Feb 3, 2023

I'm also seeing errors on every access check upon updating with the following config:

parameters:
	checkGenericClassInNonGenericObjectType: false
	earlyTerminatingMethodCalls:
		Drupal\mymodule\MyModuleJsonExceptionThrower:
			- throwInvalidJsonException
	# Sometimes Drupal gets it wrong.
	treatPhpDocTypesAsCertain: false
	level: 9
	paths:
		- web/modules/custom
		- web/themes/custom
		- tests/behat
		- scripts/
		- config/
	ignoreErrors:
	  # Drupal does not define its own arrays.
	  - '#no value type specified in iterable type array#'
	  - '#no value type specified in iterable type Drupal\\Core\\Field\\FieldItemListInterface#'
	  # Level 9 has lots of annoying offset errors.
	  - '#Cannot access offset#'
	  # Drupal screws this up. https://www.drupal.org/project/drupal/issues/3275640
	  - "#Casting to int something that's already int#"
includes:
 	- vendor/mglaman/phpstan-drupal/extension.neon
 	- vendor/thecodingmachine/phpstan-strict-rules/phpstan-strict-rules.neon
 	- vendor/phpstan/phpstan-strict-rules/rules.neon
 	- vendor/phpstan/phpstan-phpunit/extension.neon
 	- vendor/phpstan/phpstan-phpunit/rules.neon
 	- vendor/phpstan/phpstan-webmozart-assert/extension.neon
 	- vendor/phpstan/phpstan-deprecation-rules/rules.neon

@mglaman
Copy link
Owner

mglaman commented Feb 8, 2023

Odd that it would pass when the phpstan-strict-rules is removed from configuration. The rule violation appears to come from ImpossibleCheckTypeMethodCallRule.

        if (!$isAlways) {
            $method = $this->getMethod($node->var, $node->name->name, $scope);
            return [$addTip(RuleErrorBuilder::message(sprintf('Call to method %s::%s()%s will always evaluate to false.', $method->getDeclaringClass()->getDisplayName(), $method->getName(), $this->impossibleCheckTypeHelper->getArgumentsDescription($scope, $node->getArgs()))))->build()];
        } elseif ($this->checkAlwaysTrueCheckTypeFunctionCall) {
            $method = $this->getMethod($node->var, $node->name->name, $scope);
            return [$addTip(RuleErrorBuilder::message(sprintf('Call to method %s::%s()%s will always evaluate to true.', $method->getDeclaringClass()->getDisplayName(), $method->getName(), $this->impossibleCheckTypeHelper->getArgumentsDescription($scope, $node->getArgs()))))->build()];
        }

It looks like it is due to checkAlwaysTrueCheckTypeFunctionCall which is activated on level 4.

phpstan-strict-rules enables this flag, which is why it seemed to stop showing up. It enforces this flag regardless your analysis level

parameters:
	polluteScopeWithLoopInitialAssignments: false
	polluteScopeWithAlwaysIterableForeach: false
	checkAlwaysTrueCheckTypeFunctionCall: true
	checkAlwaysTrueInstanceof: true

@dpi
Copy link
Contributor

dpi commented Feb 9, 2023

1.1.29 resolves this for me, thanks

@mglaman
Copy link
Owner

mglaman commented Feb 9, 2023

thanks for verifying!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants