Problem/Motivation

In SA-CORE-2021-006, we added these lines to Drupal\media\Controller\MediaFilterController:

    self::checkCsrf($request, \Drupal::currentUser());
// ...
    if (!\Drupal::csrfToken()->validate($token, $header)) {

In a security issue, it is appropriate to use the \Drupal class, but now we should follow up and inject the csrf_token and current_user services.

As long as we are updating the constructor, we can make some additional changes (or we could defer these changes to a follow-up issue):

  1. Use constructor promotion.
  2. Use autowiring.
  3. Remove the unused property $mediaStorage, which was added in #3076773: Edit Media button within WYSIWYG should include media label for screen readers.

The argument for doing all at once is that (1) and (2) will simplify the main task of this issue, and (3) will simplify (2).

Steps to reproduce

N/A

Proposed resolution

  1. Inject the csrf_token and current_user services.
  2. (Optional, could be a follow-up) Use constructor promotion.
  3. (Optional, could be a follow-up) Use autowiring.
  4. (Optional, could be a follow-up) Remove the unused property $mediaStorage.

Since this class is marked @internal, we are allowed to make these changes (updating the signature of the constructor) without the usual steps to ensure backwards compatibility. Compare with the changes for #3076773: Edit Media button within WYSIWYG should include media label for screen readers.

Remaining tasks

User interface changes

None

Introduced terminology

None

API changes

None, since Drupal\media\Controller\MediaFilterController is marked @internal.

Data model changes

None

Release notes snippet

N/A

Issue fork drupal-3577774

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

benjifisher created an issue. See original summary.

karthik_gundu made their first commit to this issue’s fork.

karthik_gundu opened merge request !15025

benjifisher’s picture

benjifisher
he/him or they/them
Primary language English
Location Boston area
commented
Status: Active » Needs work

@karthik_gundu:

Thanks for starting to work on this issue.

It looks as though your text editor stripped the NL from the last line of the file, which caused the linting step in the CI job to fail. I am setting the issue status to NW to fix that.

If you can configure your text editor to respect the .editorconfig settings, then you will not run into this problem.

I have reviewed but not tested. It looks as though the changes in the MR handle (1) from the Proposed resolution. I would like to handle the remaining points, but that is open for discussion. If we postpone them, then we should open a follow-up issue and update the issue summary.

Since this is a refactoring issue, we might not need manual testing. If the automated tests pass, then that is probably good enough.

sivaji_ganesh_jojodae made their first commit to this issue’s fork.

sivaji_ganesh_jojodae’s picture

sivaji_ganesh_jojodae commented
Status: Needs work » Needs review

I've fixed the PHPCS issue.

Since this change is already refactoring the constructor for dependency injection, would it make sense to also adopt constructor property promotion (PHP 8+) here to reduce boilerplate?

For example:

public function __construct(
  protected RendererInterface $renderer,
  protected ContentEntityStorageInterface $media_storage,
  protected EntityRepositoryInterface $entity_repository,
  protected AccountInterface $current_user,
  protected CsrfTokenGenerator $csrf_token,
) {}

Happy to keep the current approach if we prefer to avoid mixing coding styles or keep the change minimal.

smustgrave’s picture

smustgrave commented
Status: Needs review » Needs work

Left a comment on the MR.

benjifisher’s picture

benjifisher
he/him or they/them
Primary language English
Location Boston area
commented

@sivaji_ganesh_jojodae:

Thanks for fixing the line-ending error.

Since this change is already refactoring the constructor for dependency injection, would it make sense to also adopt constructor property promotion (PHP 8+) here to reduce boilerplate?

Yes, that is #2 in the "Proposed resolution" section of the issue summary. I would like to make all the changes listed there. If you are not familiar with autowiring, then I can give you a reference.

sivaji_ganesh_jojodae’s picture

sivaji_ganesh_jojodae commented

@benjifisher, can you provide reference for autowiring? The tests are failing. Not sure if I'm missing something.

sivaji_ganesh_jojodae’s picture

sivaji_ganesh_jojodae commented
Status: Needs work » Needs review
phenaproxima’s picture

phenaproxima
he/him
Primary language English
Location Massachusetts
commented
Status: Needs review » Reviewed & tested by the community

One minor point, but it need not block commit. With my media maintainer hat on, I cheerfully approve this clean-up!

amateescu made their first commit to this issue’s fork.

  • amateescu committed b3f0ecc7 on 11.x 
    task: #3577774 Use dependency injection in Drupal\media\Controller\...

  • amateescu committed 860e7433 on main 
    task: #3577774 Use dependency injection in Drupal\media\Controller\...
amateescu’s picture

amateescu commented
Version: main » 11.x-dev
Status: Reviewed & tested by the community » Fixed

Committed and pushed 860e74337bc to main and b3f0ecc7ef1 to 11.x. Thanks!

Now that this issue is closed, review the contribution record.

As a contributor, attribute any organization that helped you, or if you volunteered your own time.

Maintainers, credit people who helped resolve this issue.

amateescu closed merge request !15025

Status: Fixed » Closed (fixed)

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