Running phpcs --standard=Drupal,DrupalPractice --extensions=php,module,inc,install,test,profile,theme,css,info,txt,md,yml shows the following errors/warnings, which should be fixed, when they are not false positives.

FILE: ...l/form_mode_manager/tests/src/Functional/FormModeManagerBase.php
----------------------------------------------------------------------
FOUND 1 ERROR AND 2 WARNINGS AFFECTING 3 LINES
----------------------------------------------------------------------
 105 | ERROR   | The @var tag must be the first tag in a member
     |         | variable comment
     |         | (Drupal.Commenting.VariableComment.VarOrder)
 171 | WARNING | t() calls should be avoided in classes, use
     |         | \Drupal\Core\StringTranslation\StringTranslationTrait
     |         | and $this->t() instead
     |         | (DrupalPractice.Objects.GlobalFunction.GlobalFunction)
 180 | WARNING | t() calls should be avoided in classes, use
     |         | \Drupal\Core\StringTranslation\StringTranslationTrait
     |         | and $this->t() instead
     |         | (DrupalPractice.Objects.GlobalFunction.GlobalFunction)
----------------------------------------------------------------------


FILE: ...form_mode_manager/tests/src/Functional/FormModeManagerUiTest.php
----------------------------------------------------------------------
FOUND 0 ERRORS AND 3 WARNINGS AFFECTING 3 LINES
----------------------------------------------------------------------
 256 | WARNING | t() calls should be avoided in classes, use
     |         | \Drupal\Core\StringTranslation\StringTranslationTrait
     |         | and $this->t() instead
     |         | (DrupalPractice.Objects.GlobalFunction.GlobalFunction)
 340 | WARNING | t() calls should be avoided in classes, use
     |         | \Drupal\Core\StringTranslation\StringTranslationTrait
     |         | and $this->t() instead
     |         | (DrupalPractice.Objects.GlobalFunction.GlobalFunction)
 361 | WARNING | t() calls should be avoided in classes, use
     |         | \Drupal\Core\StringTranslation\StringTranslationTrait
     |         | and $this->t() instead
     |         | (DrupalPractice.Objects.GlobalFunction.GlobalFunction)
----------------------------------------------------------------------


FILE: ...m_mode_manager/tests/src/Functional/FormModeManagerRouteTest.php
----------------------------------------------------------------------
FOUND 0 ERRORS AND 2 WARNINGS AFFECTING 2 LINES
----------------------------------------------------------------------
  29 | WARNING | Unused variable $node_type_test_page.
     |         | (DrupalPractice.CodeAnalysis.VariableAnalysis.UnusedVariable)
 239 | WARNING | t() calls should be avoided in classes, use
     |         | \Drupal\Core\StringTranslation\StringTranslationTrait
     |         | and $this->t() instead
     |         | (DrupalPractice.Objects.GlobalFunction.GlobalFunction)
----------------------------------------------------------------------


FILE: ...s/form_mode_theme_switcher/src/Theme/FormModeThemeNegociator.php
----------------------------------------------------------------------
FOUND 1 ERROR AND 1 WARNING AFFECTING 2 LINES
----------------------------------------------------------------------
  6 | ERROR   | [x] Use statements should be sorted alphabetically.
    |         |     The first wrong one is
    |         |     Drupal\Core\Config\ConfigFactoryInterface.
    |         |     (SlevomatCodingStandard.Namespaces.AlphabeticallySortedUses.IncorrectlyOrderedUses)
 13 | WARNING | [ ] The class short comment should describe what the
    |         |     class does and not simply repeat the class
    |         |     name (Drupal.Commenting.ClassComment.Short)
----------------------------------------------------------------------
PHPCBF CAN FIX THE 1 MARKED SNIFF VIOLATIONS AUTOMATICALLY
----------------------------------------------------------------------


FILE: ...usr/local/form_mode_manager/src/Form/FormModeManagerFormBase.php
----------------------------------------------------------------------
FOUND 2 ERRORS AND 1 WARNING AFFECTING 3 LINES
----------------------------------------------------------------------
  10 | ERROR   | [x] Use statements should be sorted alphabetically.
     |         |     The first wrong one is
     |         |     Drupal\Core\Form\ConfigFormBase.
     |         |     (SlevomatCodingStandard.Namespaces.AlphabeticallySortedUses.IncorrectlyOrderedUses)
  19 | ERROR   | [ ] Unnecessarily gendered language in a comment
     |         |     (Drupal.Commenting.GenderNeutralComment.GenderNeutral)
 139 | WARNING | [ ] t() calls should be avoided in classes, use
     |         |     \Drupal\Core\StringTranslation\StringTranslationTrait
     |         |     and $this->t() instead
     |         |     (DrupalPractice.Objects.GlobalFunction.GlobalFunction)
----------------------------------------------------------------------
PHPCBF CAN FIX THE 1 MARKED SNIFF VIOLATIONS AUTOMATICALLY
----------------------------------------------------------------------


FILE: ...mode_manager/src/Plugin/Derivative/FormModeManagerLocalTasks.php
----------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
----------------------------------------------------------------------
 9 | ERROR | [x] Use statements should be sorted alphabetically. The
   |       |     first wrong one is
   |       |     Drupal\Core\StringTranslation\StringTranslationTrait.
   |       |     (SlevomatCodingStandard.Namespaces.AlphabeticallySortedUses.IncorrectlyOrderedUses)
----------------------------------------------------------------------
PHPCBF CAN FIX THE 1 MARKED SNIFF VIOLATIONS AUTOMATICALLY
----------------------------------------------------------------------


FILE: ...usr/local/form_mode_manager/src/Plugin/EntityRoutingMap/Term.php
----------------------------------------------------------------------
FOUND 0 ERRORS AND 1 WARNING AFFECTING 1 LINE
----------------------------------------------------------------------
 22 | WARNING | The class short comment should describe what the
    |         | class does and not simply repeat the class name
    |         | (Drupal.Commenting.ClassComment.Short)
----------------------------------------------------------------------


FILE: ...usr/local/form_mode_manager/src/Plugin/EntityRoutingMap/User.php
----------------------------------------------------------------------
FOUND 0 ERRORS AND 1 WARNING AFFECTING 1 LINE
----------------------------------------------------------------------
 22 | WARNING | The class short comment should describe what the
    |         | class does and not simply repeat the class name
    |         | (Drupal.Commenting.ClassComment.Short)
----------------------------------------------------------------------


FILE: ...usr/local/form_mode_manager/src/Plugin/EntityRoutingMap/Node.php
----------------------------------------------------------------------
FOUND 0 ERRORS AND 1 WARNING AFFECTING 1 LINE
----------------------------------------------------------------------
 22 | WARNING | The class short comment should describe what the
    |         | class does and not simply repeat the class name
    |         | (Drupal.Commenting.ClassComment.Short)
----------------------------------------------------------------------


FILE: ...l/form_mode_manager/src/Plugin/EntityRoutingMap/BlockContent.php
----------------------------------------------------------------------
FOUND 0 ERRORS AND 1 WARNING AFFECTING 1 LINE
----------------------------------------------------------------------
 25 | WARNING | The class short comment should describe what the
    |         | class does and not simply repeat the class name
    |         | (Drupal.Commenting.ClassComment.Short)
----------------------------------------------------------------------


FILE: ...r/local/form_mode_manager/src/EntityFormModeManagerInterface.php
----------------------------------------------------------------------
FOUND 0 ERRORS AND 1 WARNING AFFECTING 1 LINE
----------------------------------------------------------------------
 9 | WARNING | The class short comment should describe what the class
   |         | does and not simply repeat the class name
   |         | (Drupal.Commenting.ClassComment.Short)
----------------------------------------------------------------------


FILE: ..._mode_manager/src/Controller/FormModeManagerEntityController.php
----------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
----------------------------------------------------------------------
 17 | ERROR | [x] Use statements should be sorted alphabetically. The
    |       |     first wrong one is
    |       |     Drupal\form_mode_manager\EntityRoutingMapManager.
    |       |     (SlevomatCodingStandard.Namespaces.AlphabeticallySortedUses.IncorrectlyOrderedUses)
----------------------------------------------------------------------
PHPCBF CAN FIX THE 1 MARKED SNIFF VIOLATIONS AUTOMATICALLY
----------------------------------------------------------------------


FILE: /usr/local/form_mode_manager/src/EntityRoutingMapBase.php
----------------------------------------------------------------------
FOUND 2 ERRORS AFFECTING 2 LINES
----------------------------------------------------------------------
  7 | ERROR | [x] Use statements should be sorted alphabetically. The
    |       |     first wrong one is
    |       |     Drupal\Core\Plugin\ContainerFactoryPluginInterface.
    |       |     (SlevomatCodingStandard.Namespaces.AlphabeticallySortedUses.IncorrectlyOrderedUses)
 15 | ERROR | [ ] Unnecessarily gendered language in a comment
    |       |     (Drupal.Commenting.GenderNeutralComment.GenderNeutral)
----------------------------------------------------------------------
PHPCBF CAN FIX THE 1 MARKED SNIFF VIOLATIONS AUTOMATICALLY
----------------------------------------------------------------------


FILE: /usr/local/form_mode_manager/src/FormModeManagerInterface.php
----------------------------------------------------------------------
FOUND 1 ERROR AND 1 WARNING AFFECTING 2 LINES
----------------------------------------------------------------------
   9 | WARNING | The class short comment should describe what the
     |         | class does and not simply repeat the class name
     |         | (Drupal.Commenting.ClassComment.Short)
 198 | ERROR   | Unnecessarily gendered language in a comment
     |         | (Drupal.Commenting.GenderNeutralComment.GenderNeutral)
----------------------------------------------------------------------


FILE: /usr/local/form_mode_manager/src/EntityRoutingMapInterface.php
----------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
----------------------------------------------------------------------
 6 | ERROR | [x] Use statements should be sorted alphabetically. The
   |       |     first wrong one is
   |       |     Drupal\Component\Plugin\ConfigurableInterface.
   |       |     (SlevomatCodingStandard.Namespaces.AlphabeticallySortedUses.IncorrectlyOrderedUses)
----------------------------------------------------------------------
PHPCBF CAN FIX THE 1 MARKED SNIFF VIOLATIONS AUTOMATICALLY
----------------------------------------------------------------------


FILE: ...ger/src/Routing/EventSubscriber/EnhanceEntityRouteSubscriber.php
----------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
----------------------------------------------------------------------
 8 | ERROR | [x] Use statements should be sorted alphabetically. The
   |       |     first wrong one is
   |       |     Drupal\form_mode_manager\EntityRoutingMapManager.
   |       |     (SlevomatCodingStandard.Namespaces.AlphabeticallySortedUses.IncorrectlyOrderedUses)
----------------------------------------------------------------------
PHPCBF CAN FIX THE 1 MARKED SNIFF VIOLATIONS AUTOMATICALLY
----------------------------------------------------------------------


FILE: ...mode_manager/src/Routing/EventSubscriber/FormModesSubscriber.php
----------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
----------------------------------------------------------------------
 11 | ERROR | [x] Use statements should be sorted alphabetically. The
    |       |     first wrong one is
    |       |     Drupal\form_mode_manager\EntityRoutingMapManager.
    |       |     (SlevomatCodingStandard.Namespaces.AlphabeticallySortedUses.IncorrectlyOrderedUses)
----------------------------------------------------------------------
PHPCBF CAN FIX THE 1 MARKED SNIFF VIOLATIONS AUTOMATICALLY
----------------------------------------------------------------------


FILE: /usr/local/form_mode_manager/src/MenuLinksInfo.php
----------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
----------------------------------------------------------------------
 73 | ERROR | Long array syntax must not be used in doc comment code
    |       | annotations
    |       | (Drupal.Commenting.DocCommentLongArraySyntax.DocLongArray)
----------------------------------------------------------------------

Time: 822ms; Memory: 14MB
CommentFileSizeAuthor
#59 form_mode_manager.patch11.64 KBShreyas gowda
#53 phpcs-afterapplyingMR.png73.32 KBroberttabigue
#42 afterapplylatestMR.png95.29 KBroberttabigue
#40 afterapplyMR16.png110.09 KBroberttabigue
#35 afterapplydiff.png87.34 KBroberttabigue
#35 beforeapplydiff.png368.08 KBroberttabigue
#26 interdiff_20-25.txt1.61 KBkunalgautam
#26 3291954-25.patch13.01 KBkunalgautam
#25 interdiff_20-24.txt354 byteskunalgautam
#25 3291954-24.patch12.06 KBkunalgautam
#24 3291954-23.patch17.11 KBkunalgautam
#20 3291954-20.patch11.66 KBJoel Guerreiro Borghi Filho
#2 drupal_coding_standard_issues-3291954-2.patch12.22 KBrakhi soni
#9 afterpatch.txt10.03 KBTauany Bueno
#9 aftermychanges.txt6.49 KBTauany Bueno
#11 form_mode_manager-3291954-11.patch3.36 KBJoel Guerreiro Borghi Filho
#19 3293335-8.patch9.08 KBJoel Guerreiro Borghi Filho

Issue fork form_mode_manager-3291954

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

Rakhi Soni created an issue. See original summary.

rakhi soni’s picture

rakhi soni commented
Assigned: rakhi soni » Unassigned
Status: Active » Needs review
StatusFileSize
new drupal_coding_standard_issues-3291954-2.patch12.22 KB

I have created a patch to fix the issue of Drupal Coding Standard As Per Phpcs Standard Drupal,, please review.

akshaydalvi212’s picture

akshaydalvi212 commented
Assigned: Unassigned » akshaydalvi212

Hi,
I will review this patch.

akshaydalvi212’s picture

akshaydalvi212 commented

Hi,

After applying the patch, the Drupal coding standard errors are eliminated.
so shifting the issue to Reviewed and tested by the community.

akshaydalvi212’s picture

akshaydalvi212 commented
Assigned: akshaydalvi212 » Unassigned
Status: Needs review » Reviewed & tested by the community
dww’s picture

dww
we/he/they
commented
Version: 8.x-1.x-dev » 8.x-2.x-dev
Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs reroll

Thanks!

  1. 8.x-1.x is not supported.
  2. Please re-roll this for 8.x-2.x.
Tauany Bueno’s picture

Tauany Bueno commented
Assigned: Unassigned » Tauany Bueno

hi! i'll work on it :)

tauanygb opened merge request !6

Tauany Bueno’s picture

Tauany Bueno commented
Assigned: Tauany Bueno » Unassigned
Status: Needs work » Needs review
StatusFileSize
new afterpatch.txt10.03 KB
new aftermychanges.txt6.49 KB

Hi,
I applied the patch on 8.x-2.x and fixed some other phpcs issues related to @TODO (.txt attached). Also, changed the approach to facilitate future revisions.
Changing the status to needs review.

Joel Guerreiro Borghi Filho’s picture

Joel Guerreiro Borghi Filho commented
Assigned: Unassigned » Joel Guerreiro Borghi Filho

Hi, I will review this.

Joel Guerreiro Borghi Filho’s picture

Joel Guerreiro Borghi Filho commented
Assigned: Joel Guerreiro Borghi Filho » Unassigned
StatusFileSize
new form_mode_manager-3291954-11.patch3.36 KB

I was not able to apply the #2 patch on 3291954-drupal-coding-standard branch. However, I fixed a couple more errors shown by phpcs after running phpcs (phpcs --standard=Drupal --extensions=php,module,inc,install,test,profile,theme,css,info,txt,md,yml form_mode_manager/)

This patch contains the changes I made to the code. No phpcs errors are showing.

Please review. =)

damiaosj’s picture

damiaosj commented
Assigned: Unassigned » damiaosj

I'll review this one.

damiaosj’s picture

damiaosj commented
Status: Needs review » Reviewed & tested by the community

Revised and tested through @tauanygb branch as said in the comment #9, and applied the patch of the @Joel Guerreiro Borghi Filho as said on the comment #11.

I have made some other fixes by the phpcbf and now seems fine.

Changing status to RTBC.

damiaosj’s picture

damiaosj commented
Assigned: damiaosj » Unassigned

  • dww committed 8a64a37 on 8.x-2.x 
    Issue #3291954: 'Run phpcbf' --standard=Drupal on form_mode_manager.

By...
dww’s picture

dww
we/he/they
commented
Title: Drupal Coding Standard As Per Phpcs --standrad=Drupal. » Drupal coding standard per 'phpcs --standard=Drupal'
Status: Reviewed & tested by the community » Active
Issue tags: -Needs reroll

Thanks for the flurry of activity, everyone! Nice to see so many new folks actively trying to contribute upstream. Welcome!

This was getting confusing and hard to review, since we've now got both patches and an MR. Plus, there's a mix of good changes and stuff that doesn't actually help the code in each of the approaches.

So, to hopefully simplify things, let's start over. I just ran `phpcbf` myself, reviewed the resulting diff, and pushed that to 8.x-2.x.

That leaves everything that phpcs is still complaining about. Let's start clean with a new effort on top of the latest 8.x-2.x code. Some problems with previous approaches:

  1. +++ b/modules/form_mode_theme_switcher/src/Theme/FormModeThemeNegociator.php
@@ -9,7 +9,7 @@ use Drupal\Core\Theme\ThemeNegotiatorInterface;
- * Class FormModeThemeNegociator.
+ * {@inheritDoc}

    {@inheritDoc} only works on methods or properties you're inheriting from a parent class where the thing is documented. In this case, there is no parent class, so there's no documentation to inherit.

    While this might silence the phpcs warning, it's not valid, and doesn't make the code any easier to read or understand.

    If we're going to fix these boilerplate class comments, we need to write something valid for each one. 

  2. +++ b/src/Form/FormModeManagerFormBase.php
@@ -16,7 +16,7 @@ use Symfony\Component\DependencyInjection\ContainerInterface;
- * with form mode manager and his form modes associated.
+ * with form mode manager and it's form modes associated.

    Confusingly, "it's" is short for "it is". You mean "its", the possessive form of "it". English is a terrible language. 😅 I'm so sorry.

Thanks again!
-Derek

dww’s picture

dww
we/he/they
commented

Re: #16.1: Looking at patch #2:

+++ b/src/FormModeManagerInterface.php
@@ -5,7 +5,7 @@ namespace Drupal\form_mode_manager;
+ * Interface For FormModeManagerInterface.

This is an equally unhelpful change. 😉

Thanks again,
-Derek

Joel Guerreiro Borghi Filho’s picture

Joel Guerreiro Borghi Filho commented
Assigned: Unassigned » Joel Guerreiro Borghi Filho

Hello Derek! Thanks for the warm welcome and the helpful direction on the issue!
I Will work on it!

Joel Guerreiro Borghi Filho’s picture

Joel Guerreiro Borghi Filho commented
Assigned: Joel Guerreiro Borghi Filho » Unassigned
Status: Active » Needs review
StatusFileSize
new 3293335-8.patch9.08 KB

Hello, based on the comment #16, I started out from the 8.x-2.x code, made changes and created this new patch.
Kindly review it please.

Joel Guerreiro Borghi Filho’s picture

Joel Guerreiro Borghi Filho commented
StatusFileSize
new 3291954-20.patch11.66 KB

Disregard comment #19 and patch 3293335-8.patch, I uploaded the wrong one. Sorry.

The patch for review is the 3291954-20.patch

elber’s picture

elber
Primary language Portuguese, Brazil
Location Brazil
commented
Assigned: Unassigned » elber
elber’s picture

elber
Primary language Portuguese, Brazil
Location Brazil
commented
Assigned: elber » Unassigned
Status: Needs review » Reviewed & tested by the community

Hi I applied and revised the patch #20.

I ran the command mentioned in the issue summary ($ phpcs --standard=Drupal --extensions=php,module,inc,install,test,profile,theme,css,info,txt,md,yml form_mode_manager/).

After that I saw that all coding standards was resolved.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 20: 3291954-20.patch, failed testing. View results

kunalgautam’s picture

kunalgautam commented
Issue tags: -
StatusFileSize
new 3291954-23.patch17.11 KB

More fixes of PHPCS issues.

kunalgautam’s picture

kunalgautam commented
StatusFileSize
new 3291954-24.patch12.06 KB
new interdiff_20-24.txt354 bytes
kunalgautam’s picture

kunalgautam commented
StatusFileSize
new 3291954-25.patch13.01 KB
new interdiff_20-25.txt1.61 KB
urvashi_vora’s picture

urvashi_vora commented
Title: Drupal coding standard per 'phpcs --standard=Drupal' » Fix the issues reported by PHPCS
Issue tags: +Phpcs Drupal coding standard issue
urvashi_vora’s picture

urvashi_vora commented
Assigned: Unassigned » urvashi_vora

Patch #26 failed to apply. There are several issues remaining

urvasi@urvasi-Inspiron-15-3552:/var/www/html/contribution/drupal/web/modules/contrib$ phpcs --standard=Drupal,DrupalPractice --extensions=php,module,inc,install,test,profile,theme,css,js,info,txt,md,yml,twig form_mode_manager-3291954/

FILE: ...rib/form_mode_manager-3291954/tests/src/Functional/FormModeManagerBase.php
--------------------------------------------------------------------------------
FOUND 0 ERRORS AND 2 WARNINGS AFFECTING 2 LINES
--------------------------------------------------------------------------------
 171 | WARNING | t() calls should be avoided in classes, use
     |         | \Drupal\Core\StringTranslation\StringTranslationTrait and
     |         | $this->t() instead
 180 | WARNING | t() calls should be avoided in classes, use
     |         | \Drupal\Core\StringTranslation\StringTranslationTrait and
     |         | $this->t() instead
--------------------------------------------------------------------------------


FILE: ...b/form_mode_manager-3291954/tests/src/Functional/FormModeManagerUiTest.php
--------------------------------------------------------------------------------
FOUND 0 ERRORS AND 3 WARNINGS AFFECTING 3 LINES
--------------------------------------------------------------------------------
 256 | WARNING | t() calls should be avoided in classes, use
     |         | \Drupal\Core\StringTranslation\StringTranslationTrait and
     |         | $this->t() instead
 339 | WARNING | t() calls should be avoided in classes, use
     |         | \Drupal\Core\StringTranslation\StringTranslationTrait and
     |         | $this->t() instead
 359 | WARNING | t() calls should be avoided in classes, use
     |         | \Drupal\Core\StringTranslation\StringTranslationTrait and
     |         | $this->t() instead
--------------------------------------------------------------------------------


FILE: ...orm_mode_manager-3291954/tests/src/Functional/FormModeManagerRouteTest.php
--------------------------------------------------------------------------------
FOUND 0 ERRORS AND 2 WARNINGS AFFECTING 2 LINES
--------------------------------------------------------------------------------
  29 | WARNING | Unused variable $node_type_test_page.
 238 | WARNING | t() calls should be avoided in classes, use
     |         | \Drupal\Core\StringTranslation\StringTranslationTrait and
     |         | $this->t() instead
--------------------------------------------------------------------------------


FILE: ...les/contrib/form_mode_manager-3291954/src/Form/FormModeManagerFormBase.php
--------------------------------------------------------------------------------
FOUND 0 ERRORS AND 1 WARNING AFFECTING 1 LINE
--------------------------------------------------------------------------------
 139 | WARNING | t() calls should be avoided in classes, use
     |         | \Drupal\Core\StringTranslation\StringTranslationTrait and
     |         | $this->t() instead
--------------------------------------------------------------------------------

Time: 3.71 secs; Memory: 12MB

I will work on them. Assigning it to myself. Thanks

urvashi_vora’s picture

urvashi_vora commented
Assigned: urvashi_vora » Unassigned
Status: Needs work » Needs review

Fixed all remaining issues. Committing the changes. Please review.

mahima_mathur23’s picture

mahima_mathur23 commented
Priority: Normal » Minor
Issue tags: -
elber’s picture

elber
Primary language Portuguese, Brazil
Location Brazil
commented
Status: Needs review » Reviewed & tested by the community

Hi I rewieved all the changes

I ran these commands phpcs --standard=DrupalPractice --extensions=php,module,inc,install,test,profile,theme,css,info,txt,md,yml phpcs --standard=Drupal --extensions=php,module,inc,install,test,profile,theme,css,info,txt,md,yml

PHPCS errors has been fixed.
Rebase already have done
Moving to RTBC
(Tests is failing before the issue)

dww’s picture

dww
we/he/they
commented
Status: Reviewed & tested by the community » Needs work

(Tests is failing before the issue)

Not true. Tests are failing like so:

Error: Call to undefined method Drupal\Tests\form_mode_manager\Functional\FormModeManagerUiTest::t()

That's because of these kinds of changes in the MR:

      ->pageTextContains(t('The configuration options have been saved.'));
      ->pageTextContains($this->t('The configuration options have been saved.'));

This is completely the wrong "fix" for these. We shouldn't use t() at all in tests (unless we're explicitly testing translatability). See #3133726: [meta] Remove usage of t() in tests not testing translation.

For all the changes like the above, we really want:

      ->pageTextContains('The configuration options have been saved.');

This is part of why I've come to completely hate these "fix phpcs" issues that everyone seems so excited about trying to get credit for. Y'all use automated tools to get some changes done, then make a mess of things you don't fully understand, then lots of folks pile on to "review" and "validate" the brokenness. The whole thing is a giant waste of time for almost no value. 😢

I'm tempted to close this as "won't fix"...

elber’s picture

elber
Primary language Portuguese, Brazil
Location Brazil
commented

Hi I fixed the tests that you mentioned before, but I checked the 8.x-1.x version of this module there has a lot of failing tests like this 

Testing Drupal\Tests\form_mode_manager\Functional\FormModeManagerRouteTest
EREEEEEEEEEEEEEEEEE                                               19 / 19 (100%)E

Time: 01:05.083, Memory: 12.00 MB

There were 19 errors:

1) Drupal\Tests\form_mode_manager\Functional\FormModeManagerRouteTest::testAnonymousSpecificFormModeManagerRoutes
Exception: Drupal\Tests\BrowserTestBase::$defaultTheme is required. See https://www.drupal.org/node/3083055, which includes recommendations on which theme to use.

/app/web/core/lib/Drupal/Core/Test/FunctionalTestSetupTrait.php:436
/app/web/core/tests/Drupal/Tests/BrowserTestBase.php:544
/app/web/core/tests/Drupal/Tests/BrowserTestBase.php:364
/app/web/modules/contrib/form_mode_manager/tests/src/Functional/FormModeManagerBase.php:94
/app/vendor/phpunit/phpunit/src/Framework/TestResult.php:728

2) Drupal\Tests\form_mode_manager\Functional\FormModeManagerRouteTest::testAddFormModeManagerRoutes
Exception: Drupal\Tests\BrowserTestBase::$defaultTheme is required. See https://www.drupal.org/node/3083055, which includes recommendations on which theme to use.

/app/web/core/lib/Drupal/Core/Test/FunctionalTestSetupTrait.php:436
/app/web/core/tests/Drupal/Tests/BrowserTestBase.php:544
/app/web/core/tests/Drupal/Tests/BrowserTestBase.php:364
/app/web/modules/contrib/form_mode_manager/tests/src/Functional/FormModeManagerBase.php:94
/app/vendor/phpunit/phpunit/src/Framework/TestResult.php:728

3) Drupal\Tests\form_mode_manager\Functional\FormModeManagerRouteTest::testEditFormModeManagerRoutes
Exception: Drupal\Tests\BrowserTestBase::$defaultTheme is required. See https://www.drupal.org/node/3083055, which includes recommendations on which theme to use.

/app/web/core/lib/Drupal/Core/Test/FunctionalTestSetupTrait.php:436
/app/web/core/tests/Drupal/Tests/BrowserTestBase.php:544
/app/web/core/tests/Drupal/Tests/BrowserTestBase.php:364
/app/web/modules/contrib/form_mode_manager/tests/src/Functional/FormModeManagerBase.php:94
/app/vendor/phpunit/phpunit/src/Framework/TestResult.php:728

4) Drupal\Tests\form_mode_manager\Functional\FormModeManagerRouteTest::testUserEditFormModeManagerRoutes
Exception: Drupal\Tests\BrowserTestBase::$defaultTheme is required. See https://www.drupal.org/node/3083055, which includes recommendations on which theme to use.

/app/web/core/lib/Drupal/Core/Test/FunctionalTestSetupTrait.php:436
/app/web/core/tests/Drupal/Tests/BrowserTestBase.php:544
/app/web/core/tests/Drupal/Tests/BrowserTestBase.php:364
/app/web/modules/contrib/form_mode_manager/tests/src/Functional/FormModeManagerBase.php:94
/app/vendor/phpunit/phpunit/src/Framework/TestResult.php:728

5) Drupal\Tests\form_mode_manager\Functional\FormModeManagerRouteTest::testListWithOneFormModeManagerRoutes
Exception: Drupal\Tests\BrowserTestBase::$defaultTheme is required. See https://www.drupal.org/node/3083055, which includes recommendations on which theme to use.

/app/web/core/lib/Drupal/Core/Test/FunctionalTestSetupTrait.php:436
/app/web/core/tests/Drupal/Tests/BrowserTestBase.php:544
/app/web/core/tests/Drupal/Tests/BrowserTestBase.php:364
/app/web/modules/contrib/form_mode_manager/tests/src/Functional/FormModeManagerBase.php:94
/app/vendor/phpunit/phpunit/src/Framework/TestResult.php:728

6) Drupal\Tests\form_mode_manager\Functional\FormModeManagerRouteTest::testListWithTwoFormModeManagerRoutes
Exception: Drupal\Tests\BrowserTestBase::$defaultTheme is required. See https://www.drupal.org/node/3083055, which includes recommendations on which theme to use.

/app/web/core/lib/Drupal/Core/Test/FunctionalTestSetupTrait.php:436
/app/web/core/tests/Drupal/Tests/BrowserTestBase.php:544
/app/web/core/tests/Drupal/Tests/BrowserTestBase.php:364
/app/web/modules/contrib/form_mode_manager/tests/src/Functional/FormModeManagerBase.php:94
/app/vendor/phpunit/phpunit/src/Framework/TestResult.php:728

7) Drupal\Tests\form_mode_manager\Functional\FormModeManagerRouteTest::testFormModeManagerPlugin with data set #0 ('node', 'add_form', 'node.add')
Exception: Drupal\Tests\BrowserTestBase::$defaultTheme is required. See https://www.drupal.org/node/3083055, which includes recommendations on which theme to use.

/app/web/core/lib/Drupal/Core/Test/FunctionalTestSetupTrait.php:436
/app/web/core/tests/Drupal/Tests/BrowserTestBase.php:544
/app/web/core/tests/Drupal/Tests/BrowserTestBase.php:364
/app/web/modules/contrib/form_mode_manager/tests/src/Functional/FormModeManagerBase.php:94
/app/vendor/phpunit/phpunit/src/Framework/TestResult.php:728

8) Drupal\Tests\form_mode_manager\Functional\FormModeManagerRouteTest::testFormModeManagerPlugin with data set #1 ('node', 'edit_form', 'entity.node.edit_form')
Exception: Drupal\Tests\BrowserTestBase::$defaultTheme is required. See https://www.drupal.org/node/3083055, which includes recommendations on which theme to use.

/app/web/core/lib/Drupal/Core/Test/FunctionalTestSetupTrait.php:436
/app/web/core/tests/Drupal/Tests/BrowserTestBase.php:544
/app/web/core/tests/Drupal/Tests/BrowserTestBase.php:364
/app/web/modules/contrib/form_mode_manager/tests/src/Functional/FormModeManagerBase.php:94
/app/vendor/phpunit/phpunit/src/Framework/TestResult.php:728

9) Drupal\Tests\form_mode_manager\Functional\FormModeManagerRouteTest::testFormModeManagerPlugin with data set #2 ('user', 'add_form', 'user.register')
Exception: Drupal\Tests\BrowserTestBase::$defaultTheme is required. See https://www.drupal.org/node/3083055, which includes recommendations on which theme to use.

/app/web/core/lib/Drupal/Core/Test/FunctionalTestSetupTrait.php:436
/app/web/core/tests/Drupal/Tests/BrowserTestBase.php:544
/app/web/core/tests/Drupal/Tests/BrowserTestBase.php:364
/app/web/modules/contrib/form_mode_manager/tests/src/Functional/FormModeManagerBase.php:94
/app/vendor/phpunit/phpunit/src/Framework/TestResult.php:728

10) Drupal\Tests\form_mode_manager\Functional\FormModeManagerRouteTest::testFormModeManagerPlugin with data set #3 ('user', 'edit_form', 'entity.user.edit_form')
Exception: Drupal\Tests\BrowserTestBase::$defaultTheme is required. See https://www.drupal.org/node/3083055, which includes recommendations on which theme to use.

/app/web/core/lib/Drupal/Core/Test/FunctionalTestSetupTrait.php:436
/app/web/core/tests/Drupal/Tests/BrowserTestBase.php:544
/app/web/core/tests/Drupal/Tests/BrowserTestBase.php:364
/app/web/modules/contrib/form_mode_manager/tests/src/Functional/FormModeManagerBase.php:94
/app/vendor/phpunit/phpunit/src/Framework/TestResult.php:728

11) Drupal\Tests\form_mode_manager\Functional\FormModeManagerRouteTest::testFormModeManagerPlugin with data set #4 ('user', 'admin_add', 'user.admin_create')
Exception: Drupal\Tests\BrowserTestBase::$defaultTheme is required. See https://www.drupal.org/node/3083055, which includes recommendations on which theme to use.

/app/web/core/lib/Drupal/Core/Test/FunctionalTestSetupTrait.php:436
/app/web/core/tests/Drupal/Tests/BrowserTestBase.php:544
/app/web/core/tests/Drupal/Tests/BrowserTestBase.php:364
/app/web/modules/contrib/form_mode_manager/tests/src/Functional/FormModeManagerBase.php:94
/app/vendor/phpunit/phpunit/src/Framework/TestResult.php:728

12) Drupal\Tests\form_mode_manager\Functional\FormModeManagerRouteTest::testFormModeManagerPlugin with data set #5 ('block_content', 'add_form', 'block_content.add_form')
Exception: Drupal\Tests\BrowserTestBase::$defaultTheme is required. See https://www.drupal.org/node/3083055, which includes recommendations on which theme to use.

/app/web/core/lib/Drupal/Core/Test/FunctionalTestSetupTrait.php:436
/app/web/core/tests/Drupal/Tests/BrowserTestBase.php:544
/app/web/core/tests/Drupal/Tests/BrowserTestBase.php:364
/app/web/modules/contrib/form_mode_manager/tests/src/Functional/FormModeManagerBase.php:94
/app/vendor/phpunit/phpunit/src/Framework/TestResult.php:728

13) Drupal\Tests\form_mode_manager\Functional\FormModeManagerRouteTest::testFormModeManagerPlugin with data set #6 ('block_content', 'edit_form', 'entity.block_content.edit_form')
Exception: Drupal\Tests\BrowserTestBase::$defaultTheme is required. See https://www.drupal.org/node/3083055, which includes recommendations on which theme to use.

/app/web/core/lib/Drupal/Core/Test/FunctionalTestSetupTrait.php:436
/app/web/core/tests/Drupal/Tests/BrowserTestBase.php:544
/app/web/core/tests/Drupal/Tests/BrowserTestBase.php:364
/app/web/modules/contrib/form_mode_manager/tests/src/Functional/FormModeManagerBase.php:94
/app/vendor/phpunit/phpunit/src/Framework/TestResult.php:728

14) Drupal\Tests\form_mode_manager\Functional\FormModeManagerRouteTest::testFormModeManagerPlugin with data set #7 ('taxonomy_term', 'add_form', 'entity.taxonomy_term.add_form')
Exception: Drupal\Tests\BrowserTestBase::$defaultTheme is required. See https://www.drupal.org/node/3083055, which includes recommendations on which theme to use.

/app/web/core/lib/Drupal/Core/Test/FunctionalTestSetupTrait.php:436
/app/web/core/tests/Drupal/Tests/BrowserTestBase.php:544
/app/web/core/tests/Drupal/Tests/BrowserTestBase.php:364
/app/web/modules/contrib/form_mode_manager/tests/src/Functional/FormModeManagerBase.php:94
/app/vendor/phpunit/phpunit/src/Framework/TestResult.php:728

15) Drupal\Tests\form_mode_manager\Functional\FormModeManagerRouteTest::testFormModeManagerPlugin with data set #8 ('taxonomy_term', 'edit_form', 'entity.taxonomy_term.edit_form')
Exception: Drupal\Tests\BrowserTestBase::$defaultTheme is required. See https://www.drupal.org/node/3083055, which includes recommendations on which theme to use.

/app/web/core/lib/Drupal/Core/Test/FunctionalTestSetupTrait.php:436
/app/web/core/tests/Drupal/Tests/BrowserTestBase.php:544
/app/web/core/tests/Drupal/Tests/BrowserTestBase.php:364
/app/web/modules/contrib/form_mode_manager/tests/src/Functional/FormModeManagerBase.php:94
/app/vendor/phpunit/phpunit/src/Framework/TestResult.php:728

16) Drupal\Tests\form_mode_manager\Functional\FormModeManagerRouteTest::testFormModeManagerPlugin with data set #9 ('media', 'add_form', 'entity.media.add_form')
Exception: Drupal\Tests\BrowserTestBase::$defaultTheme is required. See https://www.drupal.org/node/3083055, which includes recommendations on which theme to use.

/app/web/core/lib/Drupal/Core/Test/FunctionalTestSetupTrait.php:436
/app/web/core/tests/Drupal/Tests/BrowserTestBase.php:544
/app/web/core/tests/Drupal/Tests/BrowserTestBase.php:364
/app/web/modules/contrib/form_mode_manager/tests/src/Functional/FormModeManagerBase.php:94
/app/vendor/phpunit/phpunit/src/Framework/TestResult.php:728

17) Drupal\Tests\form_mode_manager\Functional\FormModeManagerRouteTest::testFormModeManagerPlugin with data set #10 ('media', 'edit_form', 'entity.media.edit_form')
Exception: Drupal\Tests\BrowserTestBase::$defaultTheme is required. See https://www.drupal.org/node/3083055, which includes recommendations on which theme to use.

/app/web/core/lib/Drupal/Core/Test/FunctionalTestSetupTrait.php:436
/app/web/core/tests/Drupal/Tests/BrowserTestBase.php:544
/app/web/core/tests/Drupal/Tests/BrowserTestBase.php:364
/app/web/modules/contrib/form_mode_manager/tests/src/Functional/FormModeManagerBase.php:94
/app/vendor/phpunit/phpunit/src/Framework/TestResult.php:728

18) Drupal\Tests\form_mode_manager\Functional\FormModeManagerRouteTest::testAdminRoutes with data set #0 ('form_mode_manager.admin_settings')
Exception: Drupal\Tests\BrowserTestBase::$defaultTheme is required. See https://www.drupal.org/node/3083055, which includes recommendations on which theme to use.

/app/web/core/lib/Drupal/Core/Test/FunctionalTestSetupTrait.php:436
/app/web/core/tests/Drupal/Tests/BrowserTestBase.php:544
/app/web/core/tests/Drupal/Tests/BrowserTestBase.php:364
/app/web/modules/contrib/form_mode_manager/tests/src/Functional/FormModeManagerBase.php:94
/app/vendor/phpunit/phpunit/src/Framework/TestResult.php:728

19) Drupal\Tests\form_mode_manager\Functional\FormModeManagerRouteTest::testAdminRoutes with data set #1 ('form_mode_manager.admin_setti...s_task')
Exception: Drupal\Tests\BrowserTestBase::$defaultTheme is required. See https://www.drupal.org/node/3083055, which includes recommendations on which theme to use.

/app/web/core/lib/Drupal/Core/Test/FunctionalTestSetupTrait.php:436
/app/web/core/tests/Drupal/Tests/BrowserTestBase.php:544
/app/web/core/tests/Drupal/Tests/BrowserTestBase.php:364
/app/web/modules/contrib/form_mode_manager/tests/src/Functional/FormModeManagerBase.php:94
/app/vendor/phpunit/phpunit/src/Framework/TestResult.php:728

--

There was 1 risky test:

1) Drupal\Tests\form_mode_manager\Functional\FormModeManagerRouteTest::testAnonymousSpecificFormModeManagerRoutes
This test did not perform any assertions

/app/web/core/tests/Drupal/Tests/Listeners/DrupalListener.php:127
/app/vendor/phpunit/phpunit/src/Framework/TestResult.php:452
/app/vendor/phpunit/phpunit/src/Util/PHP/AbstractPhpProcess.php:377
/app/vendor/phpunit/phpunit/src/Util/PHP/AbstractPhpProcess.php:187
/app/vendor/phpunit/phpunit/src/Framework/TestSuite.php:673
/app/vendor/phpunit/phpunit/src/TextUI/TestRunner.php:661
/app/vendor/phpunit/phpunit/src/TextUI/Command.php:144
/app/vendor/phpunit/phpunit/src/TextUI/Command.php:97

ERRORS!
Tests: 19, Assertions: 0, Errors: 19, Risky: 1.

Remaining self deprecation notices (19)

  19x: The Drupal\Tests\form_mode_manager\Functional\FormModeManagerRouteTest::$modules property must be declared protected. See https://www.drupal.org/node/2909426
    19x in DrupalListener::startTest from Drupal\Tests\Listeners

My suggestion is opening another issue to fix all the tests in 8.x-1.x branch

elber’s picture

elber
Primary language Portuguese, Brazil
Location Brazil
commented
Status: Needs work » Needs review

Hi please revise.

roberttabigue’s picture

roberttabigue commented
Status: Needs review » Reviewed & tested by the community
StatusFileSize
new beforeapplydiff.png368.08 KB
new afterapplydiff.png87.34 KB

Hi @Rakhi,

Confirmed fixed the PHPCS errors after applying plain diff file to the Form mode manager module against the 8.x-2.x-dev and with the Drupal core of 9.5.6.

Moving this now to RTBC.
Kindly refer to the screenshots attached, please.

Thank you.

avpaderno’s picture

avpaderno
he/him
Primary language Italian
Location Brescia, 🇮🇹 🇪🇺
commented
Status: Reviewed & tested by the community » Needs work 
 /**
- * Class FormModeThemeNegociator.
+ * {@inheritDoc}
  */

{@inheritDoc} is not used for a class documentation comment.

 /**
- * Interface EntityFormModeManagerInterface.
+ * {@inheritDoc}
  */
 interface EntityFormModeManagerInterface {

It is not even used for an interface documentation comment.

- * with form mode manager and his form modes associated.
+ * with form mode manager and it's form modes associated.

The correct word is its not it's, since that is the possessive for the third person it.

 /**
- * Interface FormModeManagerInterface.
+ * Interface For FormModeManagerInterface.
  */
 interface FormModeManagerInterface {

That is not the correct short description for an interface. It does not even make sense, since an interface is not an interface for itself.

 /**
- * Class BlockContent.
+ * Class For BlockContent.
  *
  * @EntityRoutingMap(

A class short description must not start with Class For, nor merely repeat the class name.

avpaderno’s picture

avpaderno
he/him
Primary language Italian
Location Brescia, 🇮🇹 🇪🇺
commented
Title: Fix the issues reported by PHPCS » Fix the issues reported by phpcs
Issue tags: +Coding standards
elber’s picture

elber
Primary language Portuguese, Brazil
Location Brazil
commented
Assigned: Unassigned » elber

I will work on it.

elber’s picture

elber
Primary language Portuguese, Brazil
Location Brazil
commented
Assigned: elber » Unassigned
Status: Needs work » Needs review

Hi I just fixed the issues reported before. Please revise

roberttabigue’s picture

roberttabigue commented
Status: Needs review » Needs work
StatusFileSize
new afterapplyMR16.png110.09 KB

Hi @elber,

After applying your MR !6 and rerunning the phpcs, a new warning was displayed.

Kindly refer to the screenshots attached, please.

Thanks.

elber’s picture

elber
Primary language Portuguese, Brazil
Location Brazil
commented
Status: Needs work » Needs review

Hi it was happening before my changes but I fixed it now, please revise.

roberttabigue’s picture

roberttabigue commented
Status: Needs review » Reviewed & tested by the community
StatusFileSize
new afterapplylatestMR.png95.29 KB

Hi @elber,

Confirmed fixed now after applying your latest MR.

Moving this now to RTBC.
Please refer the attached screenshot.

Thanks.

avpaderno’s picture

avpaderno
he/him
Primary language Italian
Location Brescia, 🇮🇹 🇪🇺
commented
Status: Reviewed & tested by the community » Needs work 
 /**
- * Class FormModeThemeNegociator.
+ * Class to define theme negotiator options.
  */
 class FormModeThemeNegociator implements ThemeNegotiatorInterface {
 
/**
- * Interface EntityFormModeManagerInterface.
+ * Interface to manage form options.
  */

Class short descriptions do not start with Class.
Interface short descriptions do not start with Interface.

- * specific FormClass, but the operation name and routes linked with her are,
+ * specific FormClass, but the operation name and routes linked with it are,

It is form class.

 /**
- * Interface FormModeManagerInterface.
+ * Load the Entity form options.
  */

The verb must use the third person singular.
Entity is misspelled, since it is not the first word in the sentence.

   * entities show up immediately. This is wrapped by Form Mode Manager to,
    * permit a more precise cache strategy and allow Form Mode Manager to,
-   * add her permissions tags.
+   * add its permissions tags.

No comma is added between to and the following verb.

 /**
- * Class BlockContent.
+ * Defines the custom block entity class.

The verb is not necessary.

 /**
- * Class Node.
+ * Class For Node.

The previous change I quoted is correct. This change is not.

 /**
- * Class Term.
+ * {@inheritDoc}
  *

{@inheritDoc} is not used in class documentation comments.

elber’s picture

elber
Primary language Portuguese, Brazil
Location Brazil
commented
Status: Needs work » Needs review
avpaderno’s picture

avpaderno
he/him
Primary language Italian
Location Brescia, 🇮🇹 🇪🇺
commented
Status: Needs review » Needs work 
  * In Entity API we have possibility to linked entity form 'handlers' to a,
- * specific FormClass, but the operation name and routes linked with her are,
+ * specific FormClass, but the operation name and routes linked with it are,

It is form class, not FormClass.

+ * specific FormClass, but the operation name and routes linked with it are,
  * very arbitrary and unpredictable specially in custom entities cases.

English does not put commas between a verb and its object. It is not My cat is, lovely. but My cat is lovely.

/**
- * Interface FormModeManagerInterface.
+ * Loads the entity form options.
  */
 interface FormModeManagerInterface {

I am not sure that is a suitable description for an interface, as an interface just defines a list of methods; it does not load entities.

 /**
- * Class Node.
+ * Create basic nodes.

Assuming that class really creates basic nodes, the verb must use the third person singular.

 /**
- * Class Term.
+ * Taxonomy term.

For a Term class, that description does not say much.

 /**
- * Class User.
+ * Class For User.

Adding For (which should not be capitalized) does not improve that documentation comment. It even makes the description wrong, since a User class is not a class for User.

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

lucienchalom’s picture

lucienchalom commented
Status: Needs work » Needs review

To make the text were we had FormClass more redable, I changed a couple of things. Looks like this now:

This plugin are used to abstract the concepts implemented by EntityPlugin.
In Entity API we have the possibility to link entity form 'handlers' to a specific form class, but the operation name and routes linked with it are very arbitrary and unpredictable specially in custom entities cases.
In that plugin you have the possibility to map operation and others useful information about entity to reduce complexity of retrieving each possible cases.

The interface description I changed to "An interface to get and return information on form modes."

And for "class node", "class term" and "class user" I changed to "Creates ___ routes".

Are those better?
Thank you for reviewing.

lucienchalom’s picture

lucienchalom commented

Changed the comments based on the review, thank you!

elber’s picture

elber
Primary language Portuguese, Brazil
Location Brazil
commented
Status: Needs review » Reviewed & tested by the community

Hi reviewed the changes.

PHPCS errors has been fixed I ran 

phpcs --standard=DrupalPractice --extensions=php,module,inc,install,test,profile,theme,css,info,txt,md,yml
phpcs --standard=Drupal --extensions=php,module,inc,install,test,profile,theme,css,info,txt,md,yml

Apaderno's suggestions were made

It sounds good to me.

moving to RTBC.

avpaderno’s picture

avpaderno
he/him
Primary language Italian
Location Brescia, 🇮🇹 🇪🇺
commented
Status: Reviewed & tested by the community » Needs work

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

sakthi_dev’s picture

sakthi_dev commented
Status: Needs work » Needs review
roberttabigue’s picture

roberttabigue commented
Status: Needs review » Reviewed & tested by the community
StatusFileSize
new phpcs-afterapplyingMR.png73.32 KB

Hi,

Reviewed the latest MR and applied it to the module, confirmed no PHPCS errors were detected.

Please see the attached file.

Moving this to RTBC.

Thank you!

avpaderno’s picture

avpaderno
he/him
Primary language Italian
Location Brescia, 🇮🇹 🇪🇺
commented
Status: Reviewed & tested by the community » Needs work
nitin_lama’s picture

nitin_lama
he/him
Location India
commented
Assigned: Unassigned » nitin_lama
nitin_lama’s picture

nitin_lama
he/him
Location India
commented
Assigned: nitin_lama » Unassigned
Status: Needs work » Needs review
elber’s picture

elber
Primary language Portuguese, Brazil
Location Brazil
commented
Status: Needs review » Reviewed & tested by the community
avpaderno’s picture

avpaderno
he/him
Primary language Italian
Location Brescia, 🇮🇹 🇪🇺
commented
Status: Reviewed & tested by the community » Needs work
Shreyas gowda’s picture

Shreyas gowda commented
StatusFileSize
new form_mode_manager.patch11.64 KB

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

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

trackleft2’s picture

trackleft2
Primary language English
Location Tucson, AZ 🇺🇸
commented
Status: Needs work » Needs review

Updated code comments with proper English.
Added string translation back to tests.

While it appears all the PHPCS errors have been resolved,
here were my results running phpunit locally on Drupal 9.5 PHP 8.1

Testing /usr/local/form_mode_manager
...............................                                   31 / 31 (100%)

Time: 03:26.627, Memory: 12.00 MB

OK (31 tests, 656 assertions)

Remaining self deprecation notices (21)

  11x: Declaring ::setUp without a void return typehint in Drupal\Tests\form_mode_manager\Functional\FormModeManagerUiTest is deprecated in drupal:9.0.0. Typehinting will be required before drupal:10.0.0. See https://www.drupal.org/node/3114724
    11x in DrupalListener::startTest from Drupal\Tests\Listeners

  3x: UiHelperTrait::drupalPostForm() is deprecated in drupal:9.1.0 and is removed from drupal:10.0.0. Use $this->submitForm() instead. See https://www.drupal.org/node/3168858
    1x in FormModeManagerRouteTest::testListWithTwoFormModeManagerRoutes from Drupal\Tests\form_mode_manager\Functional
    1x in FormModeManagerUiTest::testFormModeManagerUserOverview from Drupal\Tests\form_mode_manager\Functional
    1x in FormModeManagerUiTest::testFormModeManagerTaxonomyTermOverview from Drupal\Tests\form_mode_manager\Functional

  3x: Calling Drupal\Tests\UiHelperTrait::drupalPostForm() with $submit as an object is deprecated in drupal:9.2.0 and the method is removed in drupal:10.0.0. Use $this->submitForm() instead. See https://www.drupal.org/node/3168858
    1x in FormModeManagerRouteTest::testListWithTwoFormModeManagerRoutes from Drupal\Tests\form_mode_manager\Functional
    1x in FormModeManagerUiTest::testFormModeManagerUserOverview from Drupal\Tests\form_mode_manager\Functional
    1x in FormModeManagerUiTest::testFormModeManagerTaxonomyTermOverview from Drupal\Tests\form_mode_manager\Functional

  1x: Declaring ::setUp without a void return typehint in Drupal\Tests\form_mode_manager_examples\Functional\FormModeManagerExamplesTest is deprecated in drupal:9.0.0. Typehinting will be required before drupal:10.0.0. See https://www.drupal.org/node/3114724
    1x in DrupalListener::startTest from Drupal\Tests\Listeners

  1x: The theme 'bartik' is deprecated. See https://www.drupal.org/node/3223395#s-bartik
    1x in FormModeManagerExamplesTest::testInstalled from Drupal\Tests\form_mode_manager_examples\Functional

  1x: Adding non-existent permissions to a role is deprecated in drupal:9.3.0 and triggers a runtime exception before drupal:10.0.0. The incorrect permissions are "access toolbar". Permissions should be defined in a permissions.yml file or a permission callback. See https://www.drupal.org/node/3193348
    1x in FormModeManagerExamplesTest::testInstalled from Drupal\Tests\form_mode_manager_examples\Functional

  1x: Adding non-existent permissions to a role is deprecated in drupal:9.3.0 and triggers a runtime exception before drupal:10.0.0. The incorrect permissions are "use user.oIbE5Vtk form mode". Permissions should be defined in a permissions.yml file or a permission callback. See https://www.drupal.org/node/3193348
    1x in FormModeManagerUiTest::testFormModeManagerUserOverview from Drupal\Tests\form_mode_manager\Functional
avpaderno’s picture

avpaderno
he/him
Primary language Italian
Location Brescia, 🇮🇹 🇪🇺
commented
Status: Needs review » Needs work
avpaderno’s picture

avpaderno
he/him
Primary language Italian
Location Brescia, 🇮🇹 🇪🇺
commented

To make the last point clearer: Tests do not normally set a language for the tested pages, which means that messages in those pages are always in English. It is useless to call $this->t() or t() when the returned message is the same string passed to that method/function.

trackleft2’s picture

trackleft2
Primary language English
Location Tucson, AZ 🇺🇸
commented
Status: Needs work » Needs review
avpaderno’s picture

avpaderno
he/him
Primary language Italian
Location Brescia, 🇮🇹 🇪🇺
commented
Status: Needs review » Needs work

The issue summary reports errors/warnings for 12 files, but the MR changes 18 files. Either the issue summary must be updated, or the MR is changing more files than necessary.

avpaderno’s picture

avpaderno
he/him
Primary language Italian
Location Brescia, 🇮🇹 🇪🇺
commented
Title: Fix the issues reported by phpcs » Fix the issues reported by PHP_CodeSniffer
Issue summary: View changes
Status: Needs work » Needs review
avpaderno’s picture

avpaderno
he/him
Primary language Italian
Location Brescia, 🇮🇹 🇪🇺
commented
Status: Needs review » Needs work
elc’s picture

elc commented

Oh bother. I merged all of the PHPCS changes created as a sub-merge of #3297262: Drupal 10 compatibility fixes into it. That was probably a mistake. It is going to be a mess to merge both together as it currently reports no phpcs output.

trackleft2’s picture

trackleft2
Primary language English
Location Tucson, AZ 🇺🇸
commented
Issue summary: View changes

I've updated the summary

trackleft2’s picture

trackleft2
Primary language English
Location Tucson, AZ 🇺🇸
commented

@ELC, it was a small mistake, however, it is easy enough to fix. I'll do it.

trackleft2’s picture

trackleft2
Primary language English
Location Tucson, AZ 🇺🇸
commented
Status: Needs work » Needs review
avpaderno’s picture

avpaderno
he/him
Primary language Italian
Location Brescia, 🇮🇹 🇪🇺
commented
Status: Needs review » Needs work
trackleft2’s picture

trackleft2
Primary language English
Location Tucson, AZ 🇺🇸
commented
Status: Needs work » Needs review
trackleft2’s picture

trackleft2
Primary language English
Location Tucson, AZ 🇺🇸
commented
Status: Needs review » Needs work

Oops introduced some whitespace.

trackleft2’s picture

trackleft2
Primary language English
Location Tucson, AZ 🇺🇸
commented
Status: Needs work » Needs review

  • dww committed eadc8e36 on 8.x-2.x 
    Task #3291954: Fix issues reported by PHP_CodeSniffer:

Authored by:...
dww’s picture

dww
we/he/they
commented

Hi everyone. Thanks to everyone trying to help in here and improve the code. Sorry I let this drag on for so long. The level of noise being generated in here is very high...

@apaderno: While your desire to educate and train other contributors is very admirable, in this case, I'd recommend either:

  1. Properly using GitLab suggestions so that it's easier to adopt your proposed changes.
  2. Or maybe better yet, just push a commit to fix things. 😅 I wouldn't have cared that you both wrote and reviewed, and would have accepted an RTBC from you, regardless...

I tried to go through and save credit for everyone who seemed to be attempting to contribute here in good faith. A few "contributions" were borderline, apologies if you feel left out.

I rebased this after committing #3291950: Remove t() calls in tests not testing translation, pushed a few other fixes, merged to 8.x-2.x, and pushed commit eadc8e36fc

Calling this fixed at last! 🎉

dww closed merge request !6

dww’s picture

dww
we/he/they
commented
Status: Needs review » Fixed

Status: Fixed » Closed (fixed)

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