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

phpstan: Sitemap, Newsletter, ... #2823

Merged
merged 6 commits into from
Dec 20, 2022
Merged

phpstan: Sitemap, Newsletter, ... #2823

merged 6 commits into from
Dec 20, 2022

Conversation

sreichel
Copy link
Contributor

@sreichel sreichel commented Dec 17, 2022

Description (*)

Some small updates.

Edit: ignore some errors and remove them from baseline.

Contribution checklist (*)

  • Pull request has a meaningful description of its purpose
  • All commits are accompanied by meaningful commit messages
  • All automated tests passed successfully (all builds are green)
  • Add yourself to contributors list

@github-actions github-actions bot added Component: AdminNotification Relates to Mage_AdminNotification Component: Contacts Relates to Mage_Contacts Component: Core Relates to Mage_Core Component: Customer Relates to Mage_Customer Component: lib/Zend Component: lib/Varien Relates to lib/Varien Component: lib/* Relates to lib/* Component: Sitemap Relates to Mage_Sitemap phpstan labels Dec 17, 2022
@github-actions github-actions bot added the Mage.php Relates to app/Mage.php label Dec 18, 2022
fballiano
fballiano previously approved these changes Dec 18, 2022
ADDISON74
ADDISON74 previously approved these changes Dec 18, 2022
# Conflicts:
#	phpstan.dist.baseline.neon
@sreichel sreichel dismissed stale reviews from ADDISON74 and fballiano via ac8880e December 18, 2022 19:50
@tmotyl
Copy link
Contributor

tmotyl commented Dec 19, 2022

What is the gain from ignoring errors?
Are they all false positives?

@sreichel
Copy link
Contributor Author

@tmotyl its to much noise at the moment.

Some are false positive or go away when underlying errors are fixed.

I'll re-add them later.

@tmotyl
Copy link
Contributor

tmotyl commented Dec 20, 2022

Roger!
Thanks for taking care!

@fballiano fballiano merged commit 355cff5 into OpenMage:1.9.4.x Dec 20, 2022
@fballiano
Copy link
Contributor

can't auto merge to 20.0 because of conflicts

@sreichel sreichel deleted the phpstan/update-1 branch December 20, 2022 18:05
@sreichel
Copy link
Contributor Author

What is auto-merge?

(Btw ... if you have conflicts with baseline file ... ignore them and merge, then rebuild baseline and amend last commit.)

@fballiano
Copy link
Contributor

I meant cherry picking it to 20.0

@fballiano
Copy link
Contributor

I had an old command to regenerate the baseline, it created a .github/phpstan-baseline.neon file but now that's pushed it doesn't allow me to re-ament it.

hate this workflow

@fballiano
Copy link
Contributor

but in 20.0 there are anyway all of these files

Screenshot 2022-12-20 alle 20 12 44

so probably we can leave it as it is for now

@sreichel
Copy link
Contributor Author

sreichel commented Dec 20, 2022

but in 20.0 there are anyway all of these files

Yep. #2765 was added one day after release. Should be fixed in next release ;)

@sreichel
Copy link
Contributor Author

What is the gain from ignoring errors? Are they all false positives?

@tmotyl Maybe you can check this .... it seems neither phpstorm nor phpstan works correctly for Mage::app()->someMethod(). (this causes some of false positives)

@tmotyl
Copy link
Contributor

tmotyl commented Dec 20, 2022

@sreichel can you make an issue at https://github.com/macopedia/phpstan-magento1 with some info how to reproduce it?
When I type Mage::app()-> in phpstorm, I'm getting correct method proposals.

@sreichel
Copy link
Contributor Author

sreichel commented Dec 20, 2022

@sreichel can you make an issue at https://github.com/macopedia/phpstan-magento1 with some info how to reproduce it? When I type Mage::app()-> in phpstorm, I'm getting correct method proposals.

I'll do.

Mhh, i get no proposals for Mage::app() and also have no correct return value. E.g.

$store = Mage::app()->getStore();
if (!$store) { # false psoitive here ...

Linux, PhpStorm 2022.2.2 Build #PS-222.4167.33, built on September 15, 2022

@tmotyl
Copy link
Contributor

tmotyl commented Dec 21, 2022

PhpStorm 2022.3 here.
getStore can return null, so the check is needed

@sreichel
Copy link
Contributor Author

sreichel commented Dec 21, 2022

getStore can return null, so the check is needed

This is right, but i have no return type. For phpstan this check is always true|false ("#Negated boolean expression ...")

Edit: Cleared phpstorm cache and auto-complete works again. Will check phpstan later,

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Component: AdminNotification Relates to Mage_AdminNotification Component: Contacts Relates to Mage_Contacts Component: Core Relates to Mage_Core Component: Customer Relates to Mage_Customer Component: lib/Varien Relates to lib/Varien Component: lib/* Relates to lib/* Component: Sitemap Relates to Mage_Sitemap Mage.php Relates to app/Mage.php phpstan
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants