Our CI system checks contrib modules using PHP lint. In src/Controller/GeshiFilterConflicts, it complains about the redundant usage of GeshiFilterConflicts within the use directive and as class name (last seen using PHP 7.1.3). This breaks our builds.

Suggested fix: Remove the use directive and use the full namespace. (This should not be a problem, as it occurs once only within the file.)

CommentFileSizeAuthor
#14 2866206-remove-geshifilterconflicts-from-install.patch2.13 KBMario Steinitz
#10 2866206-remove-use-for-removed-class.patch376 bytesMario Steinitz
#2 2866206-namespace-lint-error.patch726 bytesMario Steinitz
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Mario Steinitz created an issue. See original summary.

Mario Steinitz’s picture

Mario Steinitz
Location Hong Kong Central
CreditAttribution: Mario Steinitz at SHORELESS Limited commented
FileSize
2866206-namespace-lint-error.patch726 bytes
Mario Steinitz’s picture

Mario Steinitz
Location Hong Kong Central
CreditAttribution: Mario Steinitz at SHORELESS Limited commented
Issue summary: View changes
Mario Steinitz’s picture

Mario Steinitz
Location Hong Kong Central
CreditAttribution: Mario Steinitz at SHORELESS Limited commented
yukare’s picture

yukare CreditAttribution: yukare commented
Status: Active » Needs review

I do not remember if this is not a coding standart in drupal, so let see what the testbot says.
It is: https://www.drupal.org/docs/develop/coding-standards/namespaces

Mario Steinitz’s picture

Mario Steinitz
Location Hong Kong Central
CreditAttribution: Mario Steinitz at SHORELESS Limited commented

Ok, then I suggest renaming the helper class e.g. to GeshiFilterConflictsBase. Or just using an alias with the use directive.

It's small changes. So I leave it to you. But having your module excluded from PHP lint or patching it ourself after each composer install/update is not really a nice option.

yukare’s picture

yukare CreditAttribution: yukare commented

The class just add a function, i will put the function in the same class as form.

  • Mario Steinitz authored 229ea72 on 8.x-1.x 
    Issue #2866206 by Mario Steinitz, yukare: PHP lint error in src/...
yukare’s picture

yukare CreditAttribution: yukare commented
Status: Needs review » Fixed

I removed the class, and put the function inside the form. All this code is not used anyway, it is from a part from drupal 7 that is not ported to drupal 8, there is a issue for it.
Please test it now, it will pass.

Mario Steinitz’s picture

Mario Steinitz
Location Hong Kong Central
CreditAttribution: Mario Steinitz at SHORELESS Limited commented
FileSize
2866206-remove-use-for-removed-class.patch376 bytes

Thanks for your efforts. Yet the USE directive in src/Controller/GeshiFilterConflicts.php, Line 5, is still there. This is the one causing lint to break.

Errors parsing public/modules/contrib/geshifilter/src/Controller/GeshiFilterConflicts.php
PHP Fatal error: Cannot declare class Drupal\geshifilter\Controller\GeshiFilterConflicts because the name is already in use in public/modules/contrib/geshifilter/src/Controller/GeshiFilterConflicts.php on line 11

Could you please remove that one as well? - For your convenience, I attached an according patch.

Mario Steinitz’s picture

Mario Steinitz
Location Hong Kong Central
CreditAttribution: Mario Steinitz at SHORELESS Limited commented
Status: Fixed » Needs work

  • Mario Steinitz authored 6a0670d on 8.x-1.x 
    Issue #2866206 by Mario Steinitz: PHP lint error in src/Controller/...
yukare’s picture

yukare CreditAttribution: yukare commented
Status: Needs work » Fixed

I just forgot this change in the previous commit.

Mario Steinitz’s picture

Mario Steinitz
Location Hong Kong Central
CreditAttribution: Mario Steinitz at SHORELESS Limited commented
FileSize
2866206-remove-geshifilterconflicts-from-install.patch2.13 KB

Sorry to bother again. While PHP lint is happy now, our changes caused another runtime issue:

PHP Fatal error: require(): Failed opening required 'public/modules/contrib/geshifilter/src/GeshiFilterConflicts.php' in vendor/symfony/class-loader/ApcClassLoader.php on line 110

I traced it to geshifilter.install, which still tried to use the removed class. As I'm not quite sure about the progress with the pending conflicts issue 2354511, I just commented the according lines and made the helper method listConflicts() static.

Find the suggested patch attached.

Mario Steinitz’s picture

Mario Steinitz
Location Hong Kong Central
CreditAttribution: Mario Steinitz at SHORELESS Limited commented
Status: Fixed » Needs work
yukare’s picture

yukare CreditAttribution: yukare commented
Status: Needs work » Needs review

Since it is not used, there are not tests for it. Lets see if it pass now before commit.

The last submitted patch, 10: 2866206-remove-use-for-removed-class.patch, failed testing.

Mario Steinitz’s picture

Mario Steinitz
Location Hong Kong Central
CreditAttribution: Mario Steinitz at SHORELESS Limited commented

We're using the dev version including the above patch on a current project, which is under heavy testing right now. After having commented the parts within the .install file, we did not experience any further issues due to lint and/or GeshiFilterConflicts.

  • Mario Steinitz authored 6ec138f on 8.x-1.x 
    Issue #2866206 by Mario Steinitz: PHP lint error in src/Controller/...
yukare’s picture

yukare CreditAttribution: yukare commented
Status: Needs review » Fixed

Commited, please reopen if something is still missing.

Mario Steinitz’s picture

Mario Steinitz
Location Hong Kong Central
CreditAttribution: Mario Steinitz at SHORELESS Limited commented

Thanks again you for your efforts. It's all working now.

Status: Fixed » Closed (fixed)

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