[D8] Media Revisions UI

Fix pareview.sh issues :

Review of the 8.x-1.x branch (commit a5b06a3):
Coder Sniffer has found some issues with your code (please check the Drupal coding standards). See attachment.
This automated report was generated with PAReview.sh, your friendly project application review script.

FILE: ...pal/pareviewsh/pareview_temp/src/Access/MediaRevisionAccessCheck.php
--------------------------------------------------------------------------
FOUND 6 ERRORS AFFECTING 3 LINES
--------------------------------------------------------------------------
26 | ERROR | [ ] Doc comment short description must start with a capital
| | letter
26 | ERROR | [x] Doc comment short description must end with a full stop
34 | ERROR | [ ] Doc comment short description must start with a capital
| | letter
34 | ERROR | [x] Doc comment short description must end with a full stop
43 | ERROR | [ ] Doc comment short description must start with a capital
| | letter
43 | ERROR | [x] Doc comment short description must end with a full stop
--------------------------------------------------------------------------
PHPCBF CAN FIX THE 3 MARKED SNIFF VIOLATIONS AUTOMATICALLY
--------------------------------------------------------------------------

FILE: ...reviewsh/pareview_temp/tests/src/Functional/MediaRevisionsUiTest.php
--------------------------------------------------------------------------
FOUND 8 ERRORS AFFECTING 5 LINES
--------------------------------------------------------------------------
21 | ERROR | [ ] Doc comment short description must start with a capital
| | letter
21 | ERROR | [x] Doc comment short description must end with a full stop
22 | ERROR | [ ] Missing @var tag in member variable comment
33 | ERROR | [ ] Doc comment short description must start with a capital
| | letter
33 | ERROR | [x] Doc comment short description must end with a full stop
34 | ERROR | [ ] Missing @var tag in member variable comment
59 | ERROR | [ ] Doc comment short description must start with a capital
| | letter
59 | ERROR | [x] Doc comment short description must end with a full stop
--------------------------------------------------------------------------
PHPCBF CAN FIX THE 3 MARKED SNIFF VIOLATIONS AUTOMATICALLY
--------------------------------------------------------------------------

Time: 2.61 secs; Memory: 6Mb
Reviewed on
03/09/2020 - 16:19
03/06/2020 - 23:23
03/06/2020 - 17:03
Powered by PAReview.sh, your friendly project application review script.
This service is hosted and

Same issues still exists check pareview link.

  /**
   * {@inheritDoc}
   */
  public function __construct(EntityTypeManagerInterface $entity_type_manager) {
    parent::__construct($entity_type_manager);
    $this->entityTypeManager = $entity_type_manager;
  }

The line number 26 is the line containing {@inheritDoc}. An error saying that a documentation comment must start with a capital letter or end with a period is wrong, as a documentation comment can just contain {@inheritDoc}. Those are false positives and there isn't anything benjamincizej needs to change.

Probably, the comment should contain {@inheritdoc}, with a lowercase d, but I would not change the status just for that change.

Hi @benjamincizej,

Thank you for contribution!!!
Below are some errors & warnings, you'll need to resolve as per Drupal Coding Standards.

------ -------------------------------------------------------------------------------------------- 
  Line   src/Access/MediaRevisionAccessCheck.php                                                     
 ------ -------------------------------------------------------------------------------------------- 
  37     Call to an undefined method Drupal\Core\Access\AccessResultInterface::addCacheTags().       
  46     Negated boolean expression is always false.                                                 
  68     Call to an undefined method Drupal\Core\Entity\EntityInterface::shouldCreateNewRevision().  
  77     Strict comparison using === between 'view' and 'delete' will always evaluate to false.      
  77     Strict comparison using === between 'view' and 'update' will always evaluate to false.      
 ------ -----------------------------------------------------------------------------
  Line   src/Form/MediaRevisionRevertForm.php                                                                                                                                   
 ------ ------------------------------------------------
  58     Property Drupal\media_revisions_ui\Form\MediaRevisionRevertForm::$mediaStorage (Drupal\media\MediaStorage) does not accept Drupal\Core\Entity\EntityStorageInterface.                                                                                                                  
  106    Method Drupal\media_revisions_ui\Form\MediaRevisionRevertForm::getDescription() should return Drupal\Core\StringTranslation\TranslatableMarkup but returns string.     
  113    Property Drupal\media_revisions_ui\Form\MediaRevisionRevertForm::$revision (Drupal\media\MediaInterface) does not accept Drupal\Core\Entity\EntityInterface|null.      
 ------ -------------------------------------------------------------
  Line   src/Form/MediaRevisionRevertTranslationForm.php                                                                                                                                                      
 ------ -----------------------------------------------------                                                                                                                                             
  84     Method Drupal\media_revisions_ui\Form\MediaRevisionRevertTranslationForm::getDescription() should return Drupal\Core\StringTranslation\TranslatableMarkup but returns string.                        
  112    Method Drupal\media_revisions_ui\Form\MediaRevisionRevertTranslationForm::prepareRevertedRevision() should return Drupal\media\MediaInterface but returns Drupal\Core\Entity\RevisionableInterface.  
 ------ ---------------------------------------------------------------------------

Review of the 8.x-1.x branch (commit a9d1b96): looks good to me no errors found.

1. why do you need media_revisions_ui.services.yml? Drupal core 8.9.x already defines that access check in media.services.yml? Please add a code comment to your MediaRevisionAccessCheck class.
2. media_revisions_ui.routing.yml: the 'view' operation seems wrong for deleting and reverting revisions. Can you use "delete" and "update"? Not sure if we should qualify this as security issue. It seems dangerous that if a user only has permission to view revisions that they can also delete and revert.

Setting this to "needs work" because it would be good to fix the delete/update access check so that we avoid any potential security issue.

Otherwise this looks ready!

IMO, using the wrong permission which allows users to do an action they shouldn't do is a security issue in the same way as showing account sensitive information to users without first checking they have the permission to administer users, or checking they have the permission to change their own username.

why do you need media_revisions_ui.services.yml? Drupal core 8.9.x already defines that access check in media.services.yml? Please add a code comment to your MediaRevisionAccessCheck class.

Otherwise looks good to me.

Thanks for your contribution, Benjamin!

I updated your account so you can opt into security advisory coverage now.

Here 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 lots more contributors chatting on Slack or IRC in #drupal-contribute. So, come hang out and stay involved!

Thanks, 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.

Thanks to the dedicated reviewer(s) as well.