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

Fix & enable PhanTypeInvalidRightOperandOfNumericOp #29034

Merged
merged 119 commits into from
Mar 24, 2024

Conversation

mdeweerd
Copy link
Contributor

@mdeweerd mdeweerd commented Mar 23, 2024

Fix & enable PhanTypeInvalidRightOperandOfNumericOp

This fixes almost all the PhanTypeInvalidRightOperandOfNumericOp noticies and several PhanTypeInvalidLeftOperandOfNumericOp notices.

This is built on #29006 which also fixes some of the notices (side effects/fixes at the same time).

A few notices were left unfixed to leave them for further investigation.
In particular Cronjob->$unitfrequency is defined as a string and treated as a string (escaped, etc), but used as a number in formulas. I think that should be changed to be a number.

I can add them to the phan baseline (to ignore them for now).

@mdeweerd mdeweerd force-pushed the PhanTypeInvalidRightOperandOfNumericOp branch from e953791 to 27091da Compare March 23, 2024 01:07
@mdeweerd mdeweerd marked this pull request as ready for review March 23, 2024 01:14
@mdeweerd mdeweerd force-pushed the PhanTypeInvalidRightOperandOfNumericOp branch from 26e5dbd to 463a201 Compare March 23, 2024 02:10
@mdeweerd mdeweerd force-pushed the PhanTypeInvalidRightOperandOfNumericOp branch from 463a201 to 09b931e Compare March 23, 2024 02:23
@mdeweerd
Copy link
Contributor Author

@eldy I think that you should create a PR to deprecate the fetch method and fix all exceptions there before applying it on the main branch.
It's hard to see if all exceptions for the other PRs are fixed.

@eldy
Copy link
Member

eldy commented Mar 23, 2024

@eldy I think that you should create a PR to deprecate the fetch method and fix all exceptions there before applying it on the main branch. It's hard to see if all exceptions for the other PRs are fixed.

Sorry, i wasn't aware that it was no more possible to depreciate a method without triggering errors. It seems we must also add a phan comment at each call of the deprecated function when we depreciate a new method ? No way to depreciate in an easier manner ?

@mdeweerd
Copy link
Contributor Author

Sorry, i wasn't aware that it was no more possible to depreciate a method without triggering errors. It seems we must also add a phan comment at each call of the deprecated function when we depreciate a new method ? No way to depreciate in an easier manner ?

There are three methods to disable these notices (other than fixing them):

  1. Disable the detection for all occurrences;
  2. Disable the detection on a file per file basis (through the baseline (baseline.txt).
  3. As you indicate: add an exception for each violation.

The problem with 1. and 2. is that that his disables potentially valid notices for deprecations that were already active and fixed. All occurences will still be detected through the analysis without the baseline (as available on the cti page).

If you want to introduce deprecation in a "mild" way, then maybe a change to the code before running phan & phpstan - for instance, the comment could read "deprecation-planned" and that could be replaced with "@deprecated" just for the cti report generation.

@eldy eldy merged commit 6c83239 into Dolibarr:develop Mar 24, 2024
7 checks passed
@mdeweerd mdeweerd deleted the PhanTypeInvalidRightOperandOfNumericOp branch March 24, 2024 01:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants