[D8] Media Alias Display

Thank you for applying! Remember to change status, when the project is ready to be reviewed. The current status is telling reviewers not to review it,

Have a look at the review bonus program :) https://www.drupal.org/node/1975228

I ran codesniffer on your project and it returned the following code sniffs.

❯ phpcs --standard=Drupal --extensions=php,module,inc,install,test,profile,theme,css,info,txt,md,yml --ignore=node_modules,bower_components,vendor media_alias_display

FILE: /Users/bram.driesen/DrupalContrib/web/modules/contrib/media_alias_display/src/Controller/DisplayController.php
--------------------------------------------------------------------------------------------------------------------
FOUND 1 ERROR AND 1 WARNING AFFECTING 2 LINES
--------------------------------------------------------------------------------------------------------------------
 17 | WARNING | The class short comment should describe what the class does and not simply repeat the class name
 20 | ERROR   | Doc comment is empty
--------------------------------------------------------------------------------------------------------------------

Time: 156ms; Memory: 8MB

No big issues, nor a showstopper to move on, but would be great if you can fix those small issues to have no sniffs at all.

Going a bit more through the code I noticed following things.

- In your info.yml you have the following comment

# Modelled after https://www.drupal.org/project/media_entity_download

It's not really needed to credit a module for having a look at their info.yml version ;-)

core_version_requirement: ^8.7.7 || ^9

Is there a reason to be requiring 8.7.7 specifically?

- Your README.txt only has the project name which is not helpful. Have a look here https://www.drupal.org/docs/develop/documenting-your-project/module-docu... & https://www.drupal.org/docs/develop/documenting-your-project/readme-temp... to build a meaningful readme file.

- You can also add the drupal core requirements in your composer.json (but first clarify the requirement of 8.7.7 :-) )

{
  "require": {
    "drupal/core": "^8.7.7 || ^9"
  }
}

- In your DisplayController.php class you are mixing variable naming conventions. $current_path for example is in snake case. $mediaID is no standard at all.

- In your DisplayController.php class you're using the \Drupal::logger() service, this could be injected with dependency injection.

I didn't test the module yet, but it sounds very interesting, so I'll test this on a vanilla Drupal installation in the coming days.

Your module info.yml is not compatible anymore with any Drupal core older as 8.7.7. So you need to add the core: 8.x key or bump your requirement to 8.7.7 or 8.8

What you did was good and would qualify for the bonus program.

• What follows is a quick review of the project; it doesn't mean to be complete
• For every point, I didn't make a complete list of where the code should be fixed, but an example of what is wrong in the code
• Not all the points are application stoppers; some of them describe changes that would be preferable to make

    $currentPath = $this->currentPath->getPath();
    $path = $this->request->getRequestUri();

    $alias = $this->aliasManager->getPathByAlias($currentPath);

$this->currentPath->getPath() already returns the un-aliased Drupal path.

    if ($media == NULL) {
      $this->loggerFactory->get('media_alias_display')
        ->notice("Can't find media object for " . $currentPath);
      throw new NotFoundHttpException("Can't find media object.");
    }

Instead of concatenating the strings passed to notice() the code should use placeholders.

  /**
   * @param \Drupal\Core\Entity\EntityInterface $media
   * @param string $view_mode
   * @param null $langcode
   *
   * @return array|\Symfony\Component\HttpFoundation\BinaryFileResponse|\Symfony\Component\HttpFoundation\RedirectResponse
   */
  public function view(EntityInterface $media, $view_mode = 'full', $langcode = NULL) {

A method that extends the parent's method uses {@inheritdoc} as documentation comment.

    if (strpos($path, "edit-media") !== FALSE && !$this->currentUser->isAnonymous()) {
      return new RedirectResponse('/media/' . $mid . '/edit');
    }

Why is the view method redirecting users to the edit route, when it's supposed to be invoked when the entity is viewed?

Does it need to be part of the module?

And it would be better to check for the edit media permission instead of just checking if the user isn't anonymous. It's not really an issue because the permission system would block users that don't have access to the edit page. But still it would be cleaner if you would catch it.

I edited the issue tags as per https://www.drupal.org/node/1975228.

Automated Review

[Best practice issues identified by pareview.sh / drupalcs / coder.

phpcs with Drupal standards reported no errors or warning on the latest commit to the 1.0.x branch.

Manual Review

Individual user account

[Yes: Follows] the guidelines for individual user accounts.

No duplication

[Yes: Does not cause] module duplication and/or fragmentation.

Master Branch

[Yes: Follows] the guidelines for master branch. A semantic versioning branch 1.0.x acts as the master branch for this repository.

Licensing

[Yes: Follows] the licensing requirements. No LICENSE.txt file is provided in the repository, so this meets requirements as Drupal will handle licensing.

3rd party assets/code

[Yes: Follows ] the guidelines for 3rd party assets/code. No third party assets.

README.txt/README.md

[Yes: Follows] the guidelines for in-project documentation and/or the README Template. While there are some minor white space errors in the markdown file, the contents of the README contain everything required to install and use this module. There's room for improvement here, but nothing showstopping that should block this application.

Code long/complex enough for review

[Yes: Follows] the guidelines for project length and complexity. While not overly complicated, the DisplayController.php file contains more than enough code to validate the author's understanding of a number of key concepts.

Secure code

[Yes: Meets the security requirements. ] - A clear best effort has been made to write code in a secure, or at least Drupal-y, way - I see @ placeholders for functions that provide output, and good use of existing API functions, but otherwise nothing jumps out that seems like it needs any kind of overt security / hardening.

Coding style & Drupal API usage

[List of identified issues in no particular order. Use (*) and (+) to indicate an issue importance.

1. Only the previously mentioned style issues make an appearance, none of which seem like blockers to this application. See comment #22. The decisions here are preferential and cause no issue, even if they could be enhanced.

This review uses the Project Application Review Template.

At this point, smustgrave has addressed or resolved everything that's been brought up. I'm marking this RTBC, nice work.

    if (!$file) {
      $this->loggerFactory->get('media_alias_display')
        ->notice("File id could not be loaded for " . $current_path);
      return parent::view($media, $view_mode);
    }

The string passed to notice() should use a placeholder too, like the following line.

Thank you for your contribution! I am going to update your account.

These are some recommended readings to help with excellent maintainership:

• Dries' post on Responsible maintainers
• Best practices for creating and maintaining projects
• Maintaining a drupal.org project with Git
• Commit messages - providing history and credit
• Release naming conventions.
• Helping maintainers in the issue queues

You can find more contributors chatting on the IRC #drupal-contribute channel. So, come hang out and stay involved.
Thank you, also, for your patience with the review process.
Anyone is welcome to participate in the review process. Please consider reviewing other projects that are pending review. I encourage you to learn more about that process and join the group of reviewers.

I thank all the dedicated reviewers as well.