Deprecate empty AccessInterface and remove usages

Unpostponed!

I worked on this, i am a newbie, hope i can make it into core!

thanks yesCT

ok switched

guys please i will be begging if you could for once move to github for real, stop being like that ^_^

I implemented the approach No. 1 of the description.

We haven't discussed this yet. I personally think #3 from the OP is preferable.

Anybody live here? :)

I vote for #1.

That means #4 needs a reroll.

@effulgentsia, @webchick, @alexpott, @catch and I discussed this. It's too late to remove this, but we can either deprecate it or choose to use it still for instanceof checks. Either of those things can be done after 8.0.0 as well.

We don't need to revisit this before release, so removing the tag.

Though we still should remove the usages.

As a contrib developer stumbling on this, should we still implement this interface following the documentation here: https://www.drupal.org/node/2122195 or something else?

+1 to #2 because it's confusing and hard to explain newcomers

Also argument resolving happens in strange way - no entity passed when controller does not define the same arguments as access check
PS: suppose need separate issue

Updated the issue summary. AccessInterface is pointless but has to be implemented at the moment by access checkers, which also does not make sense.

Let's get rid of it and deprecate it.

+++ b/core/lib/Drupal/Core/Routing/Access/AccessInterface.php
@@ -4,10 +4,9 @@
 /**
  * An access check service determines access rules for particular routes.
+ *
+ * @deprecated in Drupal 8.x-dev, will be removed before Drupal 9.0.
  */
 interface AccessInterface {

Should be "@deprecated in Drupal 8.2.x, will be removed before Drupal 9.0.0." It also needs an explanation of what to do instead of using this interface, with a @see linking to alternatives.

https://www.drupal.org/coding-standards/docs#deprecated

Sure, added this: "@deprecated in Drupal 8.2.x, will be removed before Drupal 9.0.0. This interface was used by access check classes but is not necessary anymore and can simply be omitted."

Patch applies, and like magic my IDE can't find any implementations of AccessInterface.

The new deprecation message is good, and the tests pass.

All the replacements look sane.

There's nothing in the issue summary about type-hinting. I can't see why you'd ever want to do instance of on AccessInterface, but how sure are we?

We are sure that the core usages of AccessInterface are cleaned up and our tests pass.

Using AccessInterface in contrib modules on access checkers is fine, since the interface is still there, it is just deprecated. There might be some very rare cases where people iterate over access checkers and validate them against AccessInterface or use an AccessInterface type hint on a method. But that should not be the case in 99.9% of Drupal 8 modules, since we cannot even come up with any reason why they would do that.

To make absolutely sure we would have to do a checkout of all drupal 8 modules with https://www.drupal.org/sandbox/greggles/1481160 and ag for AccessInterface

There might be some very rare cases where people iterate over access checkers and validate them against AccessInterface or use an AccessInterface type hint on a method. But that should not be the case in 99.9% of Drupal 8 modules, since we cannot even come up with any reason why they would do that.

Yeah this is the case I was thinking of. IMO if we commit this to 8.2.x soonish, then there's sufficient time for people's tests to break and for them to complain, then we could revert if we had to. But probably no-one will notice.

Needs a change record though I think for 'several core access checkers don't implement AccessInterface any more' - communicates that edge case and reinforces the deprecation to anyone that might have copied core code for theirs.

Change record: https://www.drupal.org/node/2731311

We must commit this to 8.1.x - otherwise people will remove AccessInterface in their contrib modules and they will stop working with Drupal 8.1.x. There is this instanceof comparision in CheckProvider.php we need to get rid of in all supported branches.

Very not keen on committing to 8.1.x - we can make it clear that people shouldn't remove the interface usage until 8.2.0 is out maybe?

I think that makes it only more confusing and painful. We could just backport the hunk where we remove the 3 lines of instanceof checks in CheckProvider.php? That would keep the change minimal on 8.1.x and contrib modules keep working with any version of 8.x.

Patch still applies to 8.2.x, not to 8.1.x. My IDE still can't find any usages of AccessInterface.

We've got our change record draft, which I just updated to refer to 8.2.x instead of 8.1.x.

Setting to RTBC and re-running the testbot.

Some work will be needed if this is to be applied to 8.1.x, though it looks like the maintainer doesn't want to.

Nice, I'd forgotten about this issue. I agree the current patch definitely should not go into 8.1.x. For 8.2.x it's an interesting question.

Regarding type hints, here is the relevant part of our BC policy:

Interfaces within non-experimental, non-test modules not tagged with either @api or @internal
Interfaces that are not tagged with either @api or @internal can be safely used as type hints. No methods will be changed or removed from these interface in a breaking way.
However, we reserve the ability to add methods to these interfaces in minor releases to support new features. When implementing the interface, module authors are encouraged to either:
Where there is a 1-1 relationship between a class and an interface, inherit from the class. Where a base class or trait is provided, use those. This way your class should inherit new methods from the class or trait when they're added.
Where the interface is implemented directly for other reasons, be prepared to add support for new methods in minor releases

This patch complies with that as written, because the interface is still in place (just deprecated). However, our BC policy doesn't cover changing the interfaces a class implements. The closest we do have is:

In general, only interfaces can be relied on as the public API.
With the exception of base classes, developers should generally assume that implementations of classes will change, and that they should not inherit from classes in the system, or be prepared to update code for changes if they do. See also more specific notes below

So access checkers would be covered by that since they are not tagged as @api.

The one change (as listed in the change record) is that for the core access checkers, they will no longer be instanceof AccessInterface, so code that was relying on that for some reason would break. And it would break in a way that just fails silently, which is actually a concern. So maybe we should limit the scope of this issue to just the deprecation, and remove the usages in 9.x (or at least discuss it in a separate followup, because the missing deprecation and the missing docs are something we want to fix soon).

Thanks for the change record; we need that either way. I think we should add to it what you should do instead, which was done in #2222719: Use parameter matching via reflection for access checks instead of pulling variables from request attributes. That issue missed the docs gate unfortunately. We still need to update the handbook documentation as well: https://www.drupal.org/node/2122195

So I'd suggest doing the deprecation, CR, and handbook updates here, and then creating a followup with the removed implementations to discuss there.

Everything you proposed sounds like an interesting approach, for Drupal 9. Because nothing you mentioned has any plan for backwards compatibility.

ArgumentsResolver long predates RouteMatch. When it was written, the only other option was to use $request->attributes directly, and hope you got a properly upcasted variable. See #2222719: Use parameter matching via reflection for access checks instead of pulling variables from request attributes.

Additionally, the "magic" of ArgumentsResolver is directly mimicking Symfony\Component\HttpKernel\Controller\ControllerResolver. If you want to pick fights with reflection to satisfy the arguments of a routing-adjacent method, start there.

My apologies that the API wasn't up to your standards. We'd appreciate help improving it. But I'd recommend reading http://xjmdrupal.org/blog/someone-worked-hard-on-it and possibly doing a bit more research on WHY we ended up with the API we have before bashing it next time.

Something seems strange to me in that discussion:

* in 8.2.x this interface is still implement by around 35 classes and extended by 1 other interface,
* all implementations of the interface include an access() method, albeit with varying parameters
* the interface extending it TestAccessCheckInterface actually includes this access() method, with no parameters

So it seems the presence of access() is a de facto characteristic of this interface, although it was removed at some point. Why not have it without any parameters ? RegisterAccessChecksPass::process() does exactly that assumption. Conversely, that same method supports adding a "method" attribute, although this has exactly 0 use case in core right now. Adding/restoring access() in the AccessInterface and removing the unused logic supporting "method" looks like it could simplify our logic a bit, and make the system more understandable. And since core uses the Interface already, this would not be a compatibility break.

Doing what is proposed in #34 results in a BC break:

class NodePreviewAccessCheck implements AccessInterface {
  public function access(AccountInterface $account, NodeInterface $node_preview) {
    // snip
  }
}

interface AccessInterface {
  public function access();
}

Results in

Fatal error: Declaration of NodePreviewAccessCheck::access() must be compatible with AccessInterface::access()  

Indeed in this case all concrete implementations must have all optional parameters, to avoid this problem.

Yes, but those implementations exist in the wild, and changing it is a BC break.

I see your point. But I'd like to push the thought expriment a bit here: since the interface does /not/ currently define a method signature, any contrib implementing such a method is not using a core API anyway, so this is a case where this is not a BC break because there is no prior definition. And core methods can be aligned to this new interface.

Just to be clear, this is more about BC policy than actual code questions, since adding the method with optional parameters allow current implementations to stand while using the updated interface: what is our guarantee with regard to adding new methods ? AIUI additions are supposed to be possible in minor releases..

If we make the change you propose, node.module and user.module will be broken. Those are vaguely important modules

Since 8.0.0 I personally know of zero interfaces that had methods added.

Deprecation would mean fixing usages in core, so we'd fix the stuff in #35. Except I don't think we should make those changes, and should instead:

@xjm #30:

The one change (as listed in the change record) is that for the core access checkers, they will no longer be instanceof AccessInterface, so code that was relying on that for some reason would break. And it would break in a way that just fails silently, which is actually a concern. So maybe we should limit the scope of this issue to just the deprecation, and remove the usages in 9.x (or at least discuss it in a separate followup, because the missing deprecation and the missing docs are something we want to fix soon).

I'd update some docs or something, but I'm not entirely clear what the replacement is.

Here's a patch which marks the interface as deprecated, and points the developer to https://www.drupal.org/node/2122195 Which is not up-to-date, but hopefully someone can update it.

fwiw #1793520: Add access control mechanism for new router system is the original discussion/issue for the access system, it was added after the initial routing patch landed, which I didn't think was a good idea at the time and still don't.

Regarding request attribute dependencies, is there a reason why the Symfony expression language component wasn't considered?

Just quickly, that was announced in November 2013, the access system was added in November 2012. So one reason was not having a time machine ;)

Off topic, but quality control would probably be a lot higher and have a faster turn around if we were using github - the main branch should really be protected against merging commits without a pull request; that way code won't be merged without unanimous approval.

github has been discussed at length elsewhere. The Drupal release cycle has been completely changed since 2012, with the intention of not merging unshippable code ever again. It's not perfect, but it's a social, not tooling issue.

@GroovyCarrot
If you want to discuss about the expression language, please add it as its own issue. Its worth for itself to discuss that.

For reference, I've just closed documentation issue #2427033: Access checking on routes page needs editing: it's an old issue (Feb 15) and the remaining item on it is duplicated by the task here to update the docs located at https://www.drupal.org/node/2122195

It does seem strange having an empty interface, but at the same time, there is an expectation on an access service -- if I don't return the right thing, I get this:

Drupal\Core\Access\AccessException: Access error in mymodule.my_access_service. Access services must return an object that implements AccessResultInterface

How is that requirement documented?

Also, the draft change record doesn't say what code should do instead of implementing the deprecated interface.

N.B. for anyone who ends up here. At least in Drupal 8.3.7, failure to implement AccessInterface in your access check class causes an exception to be thrown in \Drupal\Core\Access\CheckProvider->loadCheck(). So if you want your custom access check to work, ignore the deprecation warning for now.

So here's a use case for the having an interface. If you want to decorate an access check to implement additional functionality but play nice with others that want to do the same.

I think we should file a Drupal 9.x issue to add the access method to the interface to do #34 and require that access methods implement default arguments.

This issue could become become something that documents the change on AccessInterface because there is nothing to stop implementations already adding defaults in Drupal 8.x in order to be 9.x compatible.

Here's an example which works fine. There's no need to implement an interface, just make sure the return value is correct.

This prevents anonymous users resetting their password.

namespace Drupal\my_module\Access;

use Drupal\Core\Session\AccountInterface;
use Drupal\Core\Access\AccessResult;

class ResetPasswordAccessCheck {
  
  /**
   * Custom access check for password reset.
   *
   * @param \Drupal\Core\Session\AccountInterface $account
   *   Run access checks for this account.
   */
  public function access(AccountInterface $account) {
    
    return $account->isAnonymous() ? AccessResult::forbidden() : AccessResult::allowed();
    
  }
  
}

In alterRoutes():

    if ($route = $collection->get('user.pass')) {
      $route->setRequirement('_custom_access', '\Drupal\my_module\Access\ResetPasswordAccessCheck::access');      
    }

#55 was in response to joachim #49 and doesn't take into account #53.

It seems to me that now PHP support return types, we should keep this interface, and once Drupal 8 drops support for PHP 5, we can add a return type to the method.

I have updated the docs to reflect the fact that the interface is still required currently here:
https://www.drupal.org/docs/8/api/routing-system/access-checking-on-routes

Ended up here because I was checking the AccessInterface method signature as I thought it was a bool return type but obviously not.

Figured I'd not bother with the interface because it's not needed according to this issue... but then we end up with:

Drupal\Core\Access\AccessException: All access checks must implement AccessInterface. in Drupal\Core\Access\CheckProvider->loadCheck() (line 103 of core/lib/Drupal/Core/Access/CheckProvider.php).

Soooo looks like we have to use it but it's encourages not to?

Can we update the issue summary here? Nothing has been committed in 8.x, AccessInterface still exists in 9.2 and is not deprecated.

Also we have contradictory advice above - in #55 imclean has an example that does not use the interface and works fine. I also have tested this in contrib, from 8.8 up to 9.2 and the access check works without an interface.

But then we have mjpa in #62 who gets the Drupal\Core\Access\AccessException: All access checks must implement AccessInterface. in Drupal\Core\Access\CheckProvider->loadCheck() exception.

So I'm confused whether to use (a) AccessInterface, (b) AccessCheckInterface, or (c) neither of them.

Bumping the issue as Drupal 10 Currently doesn't support versions lower than PHP 8, should we consider adding return type as suggested in #57 or should we deprecate Interface ?

I think #53 is a way to go

We could create more than one interface. For example:

/**
 * Access rules for a route.
 */
interface RouteAccessInterface {

  public function access(AccountInterface $account, RouteMatchInterface $route): AccessResultInterface;

}

Taking this a step further, in a routing file, if the class defined in _form implements this interface, then its ::access() method could be used automatically instead of having to be defined in _custom_access.

I updated the summary.

If we are not going to deprecate AccessInterface then the @todo in its code should be replaced with an explanation of when the interface should be used.

Before we get into patches, we need to discuss what to do.

Still a problem ...

Trying to figure out what the way forward here is.

#71 points to #53

But #53 points to #34 which is going to be quite disruptive as per #35

People would then also need to add defence when parameters aren't defined otherwise static analysis tooling such as phpstan will complain on higher levels.

I like the idea in #73, funnily enough core's CsrfAccessCheck already aliases AccessInterface as RoutingAccessInterface.

Crediting for triage from #11.

Time flies... if I am not mistaken the Drupal 12.0.0 train has been already missed.

I bet is not, still makes sense to deprecate even in 12.0