This is a spin-off from #2755665: Restore original access requirement for scheduled content overview. where I used \Drupal::routeMatch()->getRawParameter('user') but I understand that this should really be using Dependency Injection.

Dependency injection is the preferred method for accessing and using services in Drupal 8 and should be used whenever possible. Rather than calling out to the global services container, services are instead passed as arguments to a constructor

I'm going to have a go at this, but thought it would be useful to separate it out to it's own issue.

CommentFileSizeAuthor
#7 2771375-7.dependency_injection_for_route_match.patch955 bytesjonathan1055
#3 2771375-3.dependency_injection_for_route_match.patch2.84 KBjonathan1055

Comments

jonathan1055 created an issue. See original summary.

jonathan1055’s picture

jonathan1055 commented
Title: Convert access » Use dependency injection for routeMatch() in ScheduledListAccess

fixed title

jonathan1055’s picture

jonathan1055 commented
Status: Active » Needs review
StatusFileSize
new 2771375-3.dependency_injection_for_route_match.patch2.84 KB

This seems to run OK on my local testing.

  • jonathan1055 committed 83032a1 on 8.x-1.x 
    Issue #2771375 by jonathan1055: Use dependency injection for routeMatch...
jonathan1055’s picture

jonathan1055 commented
Assigned: jonathan1055 » Unassigned
Status: Needs review » Fixed

Can't see any problems with this. Happy to update if anyone notices something in future.
Committed.

mr.baileys’s picture

mr.baileys
Primary language Dutch
Location 🇧🇪 (Ghent)
commented
Status: Fixed » Needs work

Sorry, was just reviewing your patch but cross-posted :(

Just minor things:

  1. +++ b/scheduler.services.yml
@@ -8,6 +8,6 @@ services:
+    arguments: ['@current_route_match', '@current_user']

    Actually, I think @current_user can be dropped here, since it is not used in the ScheduledListAccess constructor? 

  2. +++ b/src/Access/ScheduledListAccess.php
@@ -13,6 +14,23 @@ use Symfony\Component\Routing\Route;
+   * Constructs the scheuled content access check.

    Typo. I couldn't find a reference in the documentation guidelines, but it seems the consensus is the have the first doc-line of a constructor be in the form "Constructs a ... [object]."

jonathan1055’s picture

jonathan1055 commented
Status: Needs work » Needs review
StatusFileSize
new 2771375-7.dependency_injection_for_route_match.patch955 bytes

Thanks for reviewing. I am relatively new to OO and symphony framework, so very much appreciate any help I can get.

Actually, I think @current_user can be dropped here, since it is not used in the ScheduledListAccess constructor?

It is not used in the constructor, but the access() function uses $account
public function access(AccountInterface $account) {
So do we only need to specify parameters that are needed for the constructor? Does the system somehow know that AccountInterface $account is the current user?. It runs fine without that parameter so it must be OK.

mr.baileys’s picture

mr.baileys
Primary language Dutch
Location 🇧🇪 (Ghent)
commented
Status: Needs review » Reviewed & tested by the community

The $account-parameter for the access()-function is passed in by its caller (in this case AccessManager, which has the current user injected into its constructor).

So do we only need to specify parameters that are needed for the constructor?

Correct, and in this case the current user was passed as 2nd parameter to the constructor, which just ignored it. You can also use setter injection by using 'calls' instead of injecting via arguments to the constructor, for example:

typed_data_manager:
    class: Drupal\Core\TypedData\TypedDataManager
    arguments: ['@container.namespaces', '@cache.discovery', '@module_handler', '@class_resolver']
    calls:
      - [setValidationConstraintManager, ['@validation.constraint']]

  • jonathan1055 committed 7ba17a2 on 8.x-1.x 
    Issue #2771375 by jonathan1055: Remove redundant dependency injection...
jonathan1055’s picture

jonathan1055 commented
Status: Reviewed & tested by the community » Fixed

Thanks for the explanation, that makes sence. I was confused because the original patch in #2755665-4: Restore original access requirement for scheduled content overview. had the @current_user parameter, but it seems this was not required back then, because we did not have a constructor method in that patch. Yes, just checked if the order of constuctor parameters is switched it fails badly.

Committed and fixed. Thanks

Status: Fixed » Closed (fixed)

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