Drupal-mode adds drupal-phpcs to all supported modes.

[marcinant]

As in topic.

I got `drupal-mode' installed from ELPA and not enabled by default (autoloads only).

This is wrong behavior. When I open JavaScript file with js2-mode I get a bunch of drupal-phpcs errors. They should be visible only when drupal-mode is enabled in current buffer.

[marcinant]

Another thing which is wrong is that you call checker applied to all non PHP files supported by drupal-mode "drupal-phpcs" while it takes checker style from 'flycheck-phpcs-standard' variable which can be set to another value than Drupal.

So, in this way you can check javascript files with phpcs using "Wordpress" with checker called "drupal-phpcs"

[arnested]

Thank you for the issue report.

I won't be able to look into this for the next couple of weeks (on vacation). Maybe @xendk can look into it?

[xendk]

Flycheck checkers are defined globally, the issue is that the predicate for it to be used is too eager. Fixed in develop.

Checker also fixed to always use drupal/phpcs-standard.

[marcinant]

Sorry, but your solution is even more wrong!

Please reopen this bug and think about another solution. Currently I cannot even override Drupal setting at all as it's now hardcoded!

Thing is that drupal-mode adds "phpcs-drupal" checker all css and js modes.

This is not a bad thing actually. So, I was wrong that this is a bug as PHPCS supports css and js files. So, PHPCS can be applied to js/css modes while Flycheck uses PHPCS for PHP modes only.

However with previous code we could detect that our JS or CSS file is not a part of Drupal project and we could override PHPCS standard to different one customizing "flycheck-phpcs-standard" variable.

The only problem was that on Flycheck error list you could see that error is detected by (drupal-phpcs) and it could be misleading.

Now you hardcoded this to "Drupal" and for me this solution is not acceptable at all.

BTW you are partially wrong that Flycheck checkers are defined globally. They are, however we got an option to set them for each project folder separately with .dir-locals.el

[marcinant]

IMHO There is no need to define custom checker at all. Drupal-mode does good thing by adding PHPCS to js/css modes. However it should just set flycheck-phpcs-standard to Drupal if drupal-mode is enabled or leave this variable untouched. Nothing more.

Line 79 in flycheck.el from: (flycheck-add-next-checker checker 'drupal-phpcs))))) to (flycheck-add-next-checker checker 'php-phpcs)))))

And remove custom "drupal-phpcs" checker as it's unnecessary.

[xendk]

You shouldn't override drupal-phpcs standard as it's a specialized checker for Drupal non-php files. It adds itself as next-checker to all checkers of supported modes, so that it's run in addition to whatever checkers you have for those modes.

It's a custom checker rather than just modifying the standard PHPCS checker, as messing with the standard checkers is considered bad form.

If you want to run phpcs on your non-Drupal projects, feel free to borrow the drupal-mode code, however, I don't think that having drupal-mode globally enable PHPCS for non-Drupal CSS and JS files will go over well. Not all PHPCS standards include CSS and JS rules, and might it trigger spurious errors. Not to mention the performance penalty of running PHPCS on all kinds of files. That, and it's obviously out of scope for drupal-mode.

[marcinant]

Ok, you are right. I should not override Drupal checker for non-Drupal projects. However we still got a problem.

Because as I said. I just installed drupal-mode from ELPA. Not required it. It's not enabled for my js or css files. And guess what? phpcs-drupal checker is now enabled globally for my js2-mode and css-mode.

It's because you do: ''' (eval-after-load 'flycheck '(require 'drupal/flycheck)) ''' and then with lines from 75 to 79 in drupal/flycheck.el you append 'drupal-phpcs globally.

[marcinant]

Anyway I still think that there is no need to define custom drupal-phpcs checker. You should just find a solution to detect which JS or CSS file are part of Drupal project. Then set flycheck-phpcs-standard to "Drupal" and append php-phpcs to js/css modes but only when buffer is part of Drupal project.

I may be wrong but IMHO in drupal/flycheck.el lines 72-77 should go to drupal/flycheck-hook.

[xendk]

ELPA? drupal-mode is not in ELPA, as far as I know. Marmalade has 0.6.1 (@arnested decides on when to release). Melpa should have the latest.

Yes, the checker is defined on load, else flycheck wouldn't be able to use it. It shouldn't actually be used, which the latest commits attempts to ensure.

We can't use the standard php-phpcs checker, as we're actually changing it to use source-inplace, which I seem to remember was required to not have it complain about not being able to find files[] in info files.

Dynamically appending the checker when in drupal-mode isn't an option either, as it would mean we'd modify next-checker of the relevant checkers, and they're, as previously noted, defined globally. Making checkers buffer local is, apart from being confusing for users, not likely to win the heart of the flycheck community.

[marcinant]

1. MELPA - sorry.
2. So, you add drupal-phpcs checker to all supported modes by default. As much as I understand that you need to do it on load I find it annoying and wrong. I just don't want to see drupal-phpcs errors everywhere in my js/css errors list.
3. Ok. Now I get it. You are right. I just haven't noticed this option.
4. Drupal mode is minor mode which can be used for PHP and JS/CSS files as well. So, why don't you append drupal-phpcs as another checker for all supported major modes and then add this to flycheck-disabled-checkers for all these modes? And then enable checker locally only when drupal-mode is on.

#266

IMHO current situation when drupal-phpcs is always on is not acceptable. Especially when "Drupal" standard is hardcoded.

[xendk]

1. There should be no drupal-phpcs errors in non-Drupal JS/CSS files, if you're still seeing those with the latest develop branch, the fix isn't working. You're sure that drupal-mode isn't being enabled by mistake? The predicate says it should only run when drupal-mode is enabled, and the standard is set: https://github.com/arnested/drupal-mode/blob/develop/drupal/flycheck.el#L67
2. Well, I'd forgotten about it too.
3. Well, that's another way of doing it, but I'd prefer to get :predicate working instead, and leave flycheck-disabled-checkers for the users discretion. And it does seem to work on my Emacs. Running latest flycheck?

No, you're right, it should not be always on. It should only run when the buffer has drupal-mode enabled.

[marcinant]

Current version is better for me. I don't see drupal-phpcs errors for file which has no drupal-mode enabled. However I cannot see this checker when drupal-mode is enabled as well.

Another problem is what I see in fluycheck-describe-checker output:

javascript-eslint is a Flycheck syntax checker in `flycheck.el'.

  This syntax checker checks syntax in the major mode(s) `js2-mode',
  `web-mode', `js-mode', `js-jsx-mode', `js2-mode', `js2-jsx-mode',
  `js3-mode', and uses a custom predicate.  It runs the following
  checkers afterwards:

     * `drupal-phpcs'
     * `drupal-phpcs'
     * `drupal-phpcs'
     * `javascript-jscs' (maximum level `warning')

And I cannot see javascript-jscs errors at all.

[xendk]

Hmm, seems like you have the same issue that was the reason for the previous version, that is, that drupal-mode seems to not be enabled at the time of flycheck trying checking the predicate. How that can be, I'm not sure. Which flycheck version are you running? I'm using 20160224.642 from melpa. Does it work setting the flycheck-checker variable to drupal-phpcs?

Regarding the doubled checker, I see it too, though only two. Seems that the code adding drupal-phpcs to existing checkers is run multiple times, but that doesn't make sense. Maybe @arnested has some idea what could be going on.

Stupid question, but does javascript-jscs work when you set the flycheck-checker to use it?