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

[ PHP plugin ] refactoring and corrections #82

Merged
merged 6 commits into from
Mar 23, 2023
Merged

[ PHP plugin ] refactoring and corrections #82

merged 6 commits into from
Mar 23, 2023

Conversation

dedece35
Copy link
Member

@dedece35 dedece35 commented Mar 17, 2023

As mentionned on ISSUE #71, PhP rules system was not OK since our upgrade of lib versions and for SonarQube 9.9.
Done on PHP plugin :

@dedece35 dedece35 changed the title [PHP plugin] refactoring + correction PHP rules system [ISSUE 71] PHP plugin : refactoring and corrections Mar 17, 2023
@dedece35 dedece35 added 💉 bug Something isn't working php 🏗️ refactoring refactoring for best practices __PRIO_HIGH__ labels Mar 17, 2023
@dedece35 dedece35 changed the title [ISSUE 71] PHP plugin : refactoring and corrections [ PHP plugin ] refactoring and corrections Mar 17, 2023
// technical debt
Map<String, String> remediationCosts = new HashMap<>();
remediationCosts.put(AvoidSQLRequestInLoopCheck.RULE_KEY, "10min");
remediationCosts.put(AvoidFullSQLRequestCheck.RULE_KEY, "20min");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We will have to specify here if the remediation cost of a rule is different from 5 minutes? Maybe create a more modular and readable list outside the algorithm

Copy link
Member Author

@dedece35 dedece35 Mar 17, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@utarwyn,
yes you are right.
indeed, I spend a lot of time to analyse why python and php plugin didn't work since upgrade libraries version in december 2022.
I wanted find a quick solution which turn on again these two plugins for the hackathon in order to have all plugins working for developers.
But the real solution, for me, must be found after the hackathon with more time to keep a solution like done in java plugin. For example, rollback JSON files which describe this kind of information.
I created an issue for that : #80

I'm waiting for @jhertout review on next monday, to merge this branch and to have all plugins working.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the detailed answer, I understand that we have to make the plugin work for the hackaton. My comments are only open remarks and proposals, even to be applied later. I don't have yet the hindsight to validate this code, I let Julien look at it 😉

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @dedece35,

I agree with @utarwyn but as you say for the moment the important thing is to have the project to work for the hackaton. I suppose it is for the same reason you leave comments in the pom files?

The review is OK for me. I tested with the php test project and I have no problems.

Some minor things you may want to fix here (or in an other PR):

  • the rule "Prefer local variables to globals" html is in French and not well formatted
  • the rule "Use of methods for basic operations" has a compliant solution commented with // NOK

Copy link
Member Author

@dedece35 dedece35 Mar 23, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @jhertout
thank you for feedback.
Problems corrected on rule "Prefer local variables to globals". I corrected also the exact same problem on java plugin.
Capture d’écran 2023-03-23 à 23 20 23

On the other hand, I don't find / understand your problem on rule "Use of methods for basic operations".
See my capture : 2 lines with the problem and with "NOK" on each.
Capture d’écran 2023-03-23 à 22 55 32
If there was a problem, unit tests wouldn't pass. No ?

I merge this PR.
We can correct your second point on other PR.

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
@sonarqubecloud
Copy link

SonarCloud Quality Gate failed.    Quality Gate failed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot E 2 Security Hotspots
Code Smell A 8 Code Smells

89.1% 89.1% Coverage
17.9% 17.9% Duplication

@dedece35 dedece35 merged commit d1cbb44 into main Mar 23, 2023
@dedece35 dedece35 deleted the ISSUE_64_PHP branch March 23, 2023 22:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🏗️ refactoring refactoring for best practices php __PRIO_HIGH__ 💉 bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants