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

Qual: Fix phan notices #32536

Open
wants to merge 21 commits into
base: develop
Choose a base branch
from
Open

Conversation

mdeweerd
Copy link
Contributor

@mdeweerd mdeweerd commented Jan 5, 2025

Qual: Fix phan notices

Fix a set of phan notices.

@mdeweerd mdeweerd force-pushed the qual/phan.2025.1.5 branch 6 times, most recently from 07154af to 126581f Compare January 5, 2025 21:49
@mdeweerd mdeweerd marked this pull request as ready for review January 5, 2025 21:56
@mdeweerd mdeweerd force-pushed the qual/phan.2025.1.5 branch from 126581f to 3ba04e7 Compare January 6, 2025 13:49
@@ -56,14 +61,10 @@
//$search_xaxis = GETPOST('search_xaxis', 'array');
if (GETPOST('search_xaxis', 'alpha') && GETPOST('search_xaxis', 'alpha') != '-1') {
$search_xaxis = array(GETPOST('search_xaxis', 'alpha'));
} else {
Copy link
Member

Choose a reason for hiding this comment

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

Why removing this else ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The initial value is now set earlier so that $search_xaxis is always initialized for the static analysis and there is no need to repeat this (and avoids getting a notification that the value was already assigned).

Copy link
Member

@eldy eldy Jan 9, 2025

Choose a reason for hiding this comment

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

When the customreport.php page is called as a standalone page (not as an include), i do not see at wich line the $search_xaxis is already initialized.
Can you point where the init is done ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In the "else" branch where this value is not set, we have explicitly this comment:

    // $search_measures, $search_xaxis or $search_yaxis may have been defined by the parent.

It says 'may be', so the code the follows needs to be protected from unset variables.

We can observe this protection:

} elseif ($mode == 'graph' && isset($search_xaxis) && is_array($search_xaxis) && count($search_xaxis) > 1) {
setEventMessages($langs->trans("OnlyOneFieldForXAxisIsPossible"), null, 'warnings');
$search_xaxis = array(0 => $search_xaxis[0]);
}

} elseif ($mode == 'graph' && isset($search_xaxis) && is_array($search_xaxis) && count($search_xaxis) > 1) {
setEventMessages($langs->trans("OnlyOneFieldForXAxisIsPossible"), null, 'warnings');
$search_xaxis = array(0 => $search_xaxis[0]);
}

Therefore it is useless to initialize variables to "empty values" that are protected with isset anyway.

In case the protection is unsufficient, the static analysis should kick in and report it.

Copy link
Member

@eldy eldy Jan 10, 2025

Choose a reason for hiding this comment

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

I think you are right.
It's true. value can be set by parent when the page is called as an include. So should not be erased in this case.
When called as a standalone page, it may be unnitialized but if there is a protection later, we can avoid it to simplify code.
Will wait CTI green to merge with your removal.

@eldy eldy added the Discussion Some questions or discussions are opened and wait answers of author or other people to be processed label Jan 6, 2025
@mdeweerd mdeweerd force-pushed the qual/phan.2025.1.5 branch 6 times, most recently from f5ce9d7 to 28a9552 Compare January 9, 2025 15:54
@eldy eldy added PR to fix - Conflict or CI error to solve The PHP unit tests return something wrong. Check details to know what to fix or solve the conflicts. and removed Discussion Some questions or discussions are opened and wait answers of author or other people to be processed labels Jan 10, 2025
@mdeweerd mdeweerd force-pushed the qual/phan.2025.1.5 branch 6 times, most recently from 8d4ccaa to e5dba43 Compare January 15, 2025 00:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PR to fix - Conflict or CI error to solve The PHP unit tests return something wrong. Check details to know what to fix or solve the conflicts.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants