Last commit introduced some warnings in Pareview.sh: https://pareview.sh/pareview/https-git.drupal.org-project-entity_visibil...

Also the code needs to be reviewed to see if the following instruction is not missing on some files:

declare(strict_types = 1);

Also, we can consider that we are on PHP 7 and so be able to type more arguments and return of methods.

CommentFileSizeAuthor
#3 interdiff-3115174-2-3.txt3.27 KBgrimreaper
#3 entity_visibility_preview-Fix_pareview_php7-3115174-3.patch10.52 KBgrimreaper
#2 entity_visibility_preview-Fix_pareview_php7-3115174-2.patch9.01 KBPiegefull

Comments

Grimreaper created an issue. See original summary.

Piegefull’s picture

Piegefull commented
Assigned: Unassigned » grimreaper
Status: Active » Needs review
StatusFileSize
new entity_visibility_preview-Fix_pareview_php7-3115174-2.patch9.01 KB
grimreaper’s picture

grimreaper
Primary language French
Location France 🇫🇷
commented
Title: Fix Pareview, strict_types and typing arguments and return » Fix Pareview, strict_types
StatusFileSize
new entity_visibility_preview-Fix_pareview_php7-3115174-3.patch10.52 KB
new interdiff-3115174-2-3.txt3.27 KB

Hello,

Thanks for the patch.

I will create a dedicated issue for PHP7 typing.

  1. +++ b/src/Controller/PreviewMessage.php
@@ -19,7 +19,7 @@ class PreviewMessage extends ControllerBase {
+  public $entityVisibilityPreviewConditionPluginManager;

    Why changing visibility? 

  2. +++ b/src/Service/EntityVisibilityPreviewFieldHelperInterface.php
@@ -40,6 +40,7 @@ interface EntityVisibilityPreviewFieldHelperInterface {
+   *

    Correct for pareview, but not the correct order: https://www.drupal.org/docs/develop/standards/api-documentation-and-comm...

    I agree that it can be hard to find this piece of documentation of standards.

    I made the change.

You forgot some files without the "declare" and one inheritDoc.

To avoid forgetting inheritDoc, use PhpStorm search feature :).

To search for files without a string, I was not able to find that in PhpStorm, but a simple https://stackoverflow.com/questions/1748129/how-do-i-find-files-that-do-..., do the job.

I have also added jsonapi_extras in require-dev to avoid drupal-check validation errors.

I will handle param and return typing in another issue.

Waiting for tests to execute before merging.

  • Piegefull authored 304f50b on 8.x-1.x 
    Issue #3115174 by Piegefull, Grimreaper: Fix Pareview, strict_types
grimreaper’s picture

grimreaper
Primary language French
Location France 🇫🇷
commented
Assigned: grimreaper » Unassigned
Status: Needs review » Fixed

Status: Fixed » Closed (fixed)

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