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.

Sign up for GitHub

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

Jump to bottom

refactor: fix PHPStan errors #9125

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

refactor: fix PHPStan errors #9125

wants to merge 10 commits into from

Conversation

pawelkg
Copy link
Contributor

@pawelkg pawelkg commented Aug 15, 2024 •
edited by kenjis
Loading

Description
Fix errors from phpstan-baseline.php

Checklist:

  • Securely signed commits
  • Component(s) with PHPDoc blocks, only if necessary or adds value
  • Unit testing, with >80% coverage
  • User guide updated
  • Conforms to style guide

@samsonasik
Copy link
Member

please create multiple PRs for per-directory basis to make easier to review, also, better fix per error identifier instead of try to fix everything, then you can rebase when one of PRs merged.

@kenjis
Copy link
Member

kenjis commented Aug 16, 2024 •
edited
Loading

Sorry, we cannot review too big PRs like this (changes: +413 −778). Your one commit is too big and having a lot of different changes,
and the commit messages do not make sense like "fix another PHPUnit DatabaseLive errors part2".
One commit should be for one change, and the commit message should be more meaningful.

Now we don't have detailed rules on commits and its messages. But atomic commit is recommended. Keep your commits atomic. One commit for one change.

Read carefully: https://github.com/codeigniter4/CodeIgniter4/blob/develop/contributing/workflow.md#committing

And could you please make multiple very small PRs, and change (fix) one thing in one PR?

kenjis
kenjis reviewed Aug 16, 2024
View reviewed changes
system/Config/AutoloadConfig.php
*
* @var list<string>
*/
public $helpers = [];
Copy link
Member

Choose a reason for hiding this comment

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

Why did you add the property?

kenjis
kenjis reviewed Aug 16, 2024
View reviewed changes
system/Autoloader/Autoloader.php
Comment on lines -383 to +392
if (! method_exists(InstalledVersions::class, 'getAllRawData')) {
preg_match(
'/\s*"plugin-api-version": "(?\'version\'\d+\.\d+\.\d+)"/m',
file_get_contents(__DIR__ . '/../../composer.lock'),
$matches
);

if (version_compare($matches['version'], '2.0.14', '<')) {
Copy link
Member

Choose a reason for hiding this comment

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

Why did you change the code?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This fix phpstan error:
Call to function method_exists() with 'Composer\InstalledVersions' and 'getAllRawData' will always evaluate to true.
But I think, that better way will be to remove this check and check composer version during composer install and composer update...

Copy link
Member

Choose a reason for hiding this comment

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

Okay, I got why. But your code seems to take time more.
The PHPStan error is inevitable. Because we use the latest composer command.
So I think the error should be suppressed by @phpstan-ignore-next-line or @phpstan-ignore-line.

kenjis
kenjis reviewed Aug 16, 2024
View reviewed changes
system/Autoloader/FileLocator.php
$namespace = 0;
$namespace = '';
Copy link
Member

Choose a reason for hiding this comment

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

I don't know why the original code is $namespace = 0;.
If $namespace is set to 0, is the case is just a bug?

Please make another PR to fix this issue.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

After changing 151 line in this file it's failing:

  1. CodeIgniter\Autoloader\FileLocatorCachedTest::testGetClassNameFromClassFile
    Failed asserting that two strings are identical.
    --- Expected
    +++ Actual
    @@ @@
    -'CodeIgniter\Autoloader\FileLocatorTest'
    +'0\CodeIgniter\Autoloader\FileLocatorTest'

@kenjis kenjis added the needs rework Changes requested by reviewer that are still pending label Aug 16, 2024
@pawelkg
Copy link
Contributor Author

pawelkg commented Aug 16, 2024

Ok. I will prepare separate PRs with smaller, internally related scopes.

@pawelkg pawelkg mentioned this pull request Aug 17, 2024
Open
5 tasks
@github-actions github-actions bot added the stale Pull requests with conflicts label Aug 29, 2024
@github-actions GitHub Actions
Copy link

👋 Hi, @pawelkg!

We detected conflicts in your PR against the base branch 🙊
You may want to sync 🔄 your branch with upstream!

Ref: Syncing Your Branch

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kenjis kenjis kenjis left review comments

Assignees
No one assigned
Labels
needs rework Changes requested by reviewer that are still pending stale Pull requests with conflicts
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@pawelkg @samsonasik @kenjis