Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Additional (code standard?) rules are running by default #3222

Closed
shaal opened this issue Apr 23, 2020 · 9 comments · Fixed by #3311
Closed

Additional (code standard?) rules are running by default #3222

shaal opened this issue Apr 23, 2020 · 9 comments · Fixed by #3311
Labels

Comments

@shaal
Copy link
Contributor

shaal commented Apr 23, 2020

Bug Report

Subject Details
Rector version v0.7.16 (invoke vendor/bin/rector --version)
Installed as prefixed Rector PHAR

Minimal PHP Code Causing Issue

In this example a docblock is being removed, because there's an empty docblock after it:

-/**
- * This docblock is being deleted when there's an empty docblock after it.
- */
-
-/**
- * 
- */
-

https://getrector.org/demo/9dbe8d1d-b062-4b16-97d2-605769834cbc#result

In this example empty comment lines are being removed, and 2 lines are being removed and added back the same way:


 /**
  * The empty line after this docblock is being deleted.
- */
-
+ */
 /**
  * Another dockblock.
- * 
+ *  
  *
  * first_function()
- * 
+ *  
  *
- * second_function()
- * third_function()
+ * second_function()
+ * third_function()
  * fourth_function()
- */
-
+ */

https://getrector.org/demo/0737a1e7-0b83-4e50-9e64-72f2e33476c8#result

Expected Behaviour

I'm expecting rector to run only the rules that are set in rector.yml, and if it's not explicitly called it shouldn't run.

@shaal
Copy link
Contributor Author

shaal commented Apr 23, 2020

@TomasVotruba Can you please help us?
This became a major/urgent bug for us.

We are running Drupal Rector against 10,000 projects (Drupal modules), to help them upgrade their code to the upcoming Drupal 9.

The Rector rules we implemented can already help 3,000 of these projects, as you can see in this list (rightmost column):
https://dev.acquia.com/drupal9/deprecation_status?name=&result=&error=&type=&group=&plan=&patch=available&op=Submit

But because Rector is running these additional code style rules, we cannot send the patches to all these projects that could benefit from it.

Here's an example with a mix of expected Rector rules and unexpected code style rules -
(lines 91, 99-260, 270, 278-395)
https://dispatcher.drupalci.org/job/phpstan/1462/artifact/phpstan-results/google_analytics.2.4.rector.patch/*view*/

(same file uploaded to github)
google_analytics-drupal-module--rector-generated-patch.txt

@TomasVotruba
Copy link
Member

If you provide failing PR with minimal test case (1 file), it makes it much simpler to fix.

@shaal
Copy link
Contributor Author

shaal commented Apr 23, 2020

Thank you.
These are the minimal test cases I created in main description of this issue:

Please let me know if there's a better way to provide test cases.

@TomasVotruba
Copy link
Member

I don't mean demo, but pull-request to code here. So continuous integration unit test are failing.

@shaal
Copy link
Contributor Author

shaal commented Apr 23, 2020

@TomasVotruba Do you mean creating something similar to this PR?
https://github.com/rectorphp/rector/pull/2943/files#diff-3ac972f8fab9c120f8727f6c02594334

If yes, how do I know were it should be located? I don't know what rule is creating these changes and where might it be called from.

@TomasVotruba
Copy link
Member

Yes. The location is good question. We don't have such tests here AFAIK, just for particular @annotatoin. But I'd look somewhere in packages/better-php-doc-parser/tests

@damontgomery
Copy link

@shaal & @TomasVotruba, I created #3250 to try and start this process.

The AbstractRectorTestCase tests didn't seem like they were the way to go because they only ran a single rule (which I can't identify) and they also didn't report failures the way the one I submitted did.

I may have time next week to look at this more.

@tedbow
Copy link

tedbow commented Apr 24, 2020

Is this related to #3212 and #2994?

shaal pushed a commit to shaal/rector that referenced this issue Apr 27, 2020
@TomasVotruba
Copy link
Member

@tedbow No, that is spacing of class annotations. This one is about multiple comments.

It was introduced in this PR: #2775
There will be also a bug, in BetterStandardPrinter and update* method

TomasVotruba added a commit that referenced this issue May 4, 2020
TomasVotruba added a commit that referenced this issue May 4, 2020
TomasVotruba added a commit that referenced this issue May 4, 2020
TomasVotruba added a commit that referenced this issue May 4, 2020
TomasVotruba added a commit that referenced this issue May 4, 2020
TomasVotruba added a commit that referenced this issue May 4, 2020
TomasVotruba added a commit that referenced this issue May 4, 2020
TomasVotruba added a commit that referenced this issue May 4, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants