Invoke drupal_alter() for initial code snippet injected into export files. [#2099899]

Related to #1231118: Generate code according to coding standards, but I don't feel duplicated by.

Since Features seems to be reliant on the export code provided by whatever modules are utilized in the generation of a given Feature, this opens up the possibility of a whole deluge of errors kicked up when putting the code through PHP Code Sniffer.

I would propose there ought to be a checkbox option in the "Advanced usage" (or perhaps just general Settings): 'Insert PHP Code Sniffer directives to ignore generated code.'

If this is enabled, Features would insert
// @codingStandardsIgnoreStart
(at the beginning of a file) and
// @codingStandardsIgnoreEnd
at the end.

I'm thinking this would only be in the individual .inc files. This would not apply to the .install or .module as custom code can be in here which ought to be picked up by phpcs.

One can modify the phpcs command line to not scan .inc files, but I have a Feature module with a custom Views plugin associated and it is an .inc file..

Comment	File	Size	Author
#9	features-hook_features_export_header_alter-2099899-8.patch	2.13 KB	reubenavery
#9
#8	features-hook_features_export_header_alter-2099899-8.patch	2.13 KB	reubenavery
#8
#5	features-hook_features_export_header_alter-2099899-5.patch	2.4 KB	reubenavery
#5

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Comment #1

hefox CreditAttribution: hefox commented 30 September 2013 at 17:57

If exported code has coding style issues, it should be fixed instead of ignored imo

Comment #2

reubenavery CreditAttribution: reubenavery commented 30 September 2013 at 18:11

yes of course it should be, but we need to allow for edge cases where it's not and to offer users a way around a potential blocker for their workflow.

this is no different than the 'allow conflicts' option.

Comment #3

hefox CreditAttribution: hefox commented 1 October 2013 at 04:56

Allow conflict mode is for stuff like recreating features TMK; I don't see the connection, or at least not enough to justify the extra messyness that adding it and it's option will add. Having an alter for the rendered export, which has been discussed in other issues, seems more worth it.

Comment #4

reubenavery CreditAttribution: reubenavery commented 1 October 2013 at 13:49

The allow overrides option came about from a similar situation. It used to be that two silly features trying to assert the same user permission (or whatever) would block each other from being enabled, even if both settings were identical. This unfairly placed a blockage in development until the so-called conflict was resolved.

Likewise, we have a similar condition here that can throw blocks into a development path. "I'd like to have code sniffing engaged but if I put it on our Features, it blows up with hundreds of errors." Being in a typical dev project, we're behind schedule and overworked anyway and now we're supposed to divert even more time in solving some contrib module's export formatting in order to please phpcs? Sorry, can't do that now, maybe later on. For now, well I guess we will just have to disable phpcs codesniff on our features and hope that we can catch the mistakes in pull request reviews?

The alter hook idea is a good and simple approach, however. I will put something together and sandbox it.

Comment #5

reubenavery CreditAttribution: reubenavery commented 7 October 2013 at 18:24

Status:

Active

» Needs review

File	Size
features-hook_features_export_header_alter-2099899-5.patch	2.4 KB

Here's my little patch. I'm introducing a new hook_features_export_header_alter() to allow alteration of the initial code snippet in exported module components' files.