Support PHP 8.5 #61
Support PHP 8.5 #61
Conversation
sukhwinder33445
force-pushed
the
support-php-85
branch
from
4322c75 to
7354db6
Compare
sukhwinder33445
force-pushed
the
support-php-85
branch
from
52f1357 to
415ad2b
Compare
sukhwinder33445
force-pushed
the
support-php-85
branch
from
2fc2357 to
ff553e0
Compare
sukhwinder33445
force-pushed
the
support-php-85
branch
2 times, most recently
from
6408fa8 to
d67ed2a
Compare
There was a problem hiding this comment.
Since we recently discussed pull requests and commits, let’s already start improving. I’ll provide some examples for the commits in this PR. The commit message should state what was done, and the description should justify it. I also recommend limiting both the commit message and its description to 72/80 characters per line. In my view, this is a reasonable and descriptive length that still provides excellent readability—compared to the traditional 50/72-character guideline originating from the Linux kernel conventions.
PHP 8.4: Change implicit nullable type declaration to explicit
Since PHP 8.4 implicitly nullable parameter types are deprecated.
PHP 8.4: Fix `PriorityQueue` return type declaration compatibility
Since PHP 8.4 `SplPriorityQueue::insert()` has a tentative return of `true`.
There are two more points I would like to address:
c473720
In case anyone else is reading this, I would first like to clarify that I wrote the description of the scanDirectories change. Anyway, here’s the thing:
You and @BastianLedererIcinga added references to merge requests for the change, which should be removed everywhere. First, if we were to add references to merge requests, I would prefer to use the short commit hash of the merge commit so that it is understandable in both GitHub and non-GitHub contexts (such as CLI, IDE). Second, it's not the new workflows that make the definition of scanDirectories superfluous. If we omitted them with the old workflows, everything would have continued to work because PHPStan uses the Composer autoloader by default. They were only introduced to simplify local testing without the shortcomings in mind I mentioned in the commit description. Even though the commit message may not be perfect, the description clearly addresses the issue. It's not about the new workflow, but about the fact that it shouldn't exist at all.
d67ed2a
This goes beyond fixing warnings in our unit tests. Without context, this code simply looks wrong, as it yields warnings in every version of PHP. So why didn't it blew up already, or rather, why do we have this code in the first place? One of the first things we do in Icinga Web is change the error reporting to really get exceptions for everything. That's why this code works in Icinga Web but not in the library’s unit tests. Essentially, you are fixing a programming error. So please go ahead and change the commit message and description accordingly. Also, it might make sense to have PHPUnit report warnings as failures:
- @lippserd will open an issue in our GitHub Actions repo to clarify whether PHPUnit should report warnings as failures.
|
I think it is also worth mentioning that no special changes are required with regard to PHP 8.5. |
Since PHP 8.4 implicitly nullable parameter types are deprecated.
…ibility Since PHP 8.4 `SplPriorityQueue::insert()` has a tentative return of `true`.
`scanDirectories` was set to `/usr/share/icinga-php` to simplify local testing, as both the Icinga PHP Library and Icinga PHP Thirdparty are installed there in our development environment. Our individual PHP library components, which make up these libraries, are self-contained, as they define all necessary dependencies themselves. This then requires testing with exactly these dependencies instead of an arbitrary folder that could contain anything, e.g., dependencies in unexpected versions or dependencies that have not yet been defined. For remote and local testing, `composer install` must be executed and tests must be performed with exactly the resulting dependencies. Since PHPStan uses the Composer autoloader by default, if available, `scanDirectories` does not need to be defined at all.
…value is unset
Dynamic property access ($row->{$column}) caused PHP warnings when `$value` was
undefined, resulting in attempts to read non-existent properties. Updated the
logic to use null coalescin, which safely returns `null` for missing properties
and prevents these warnings.
sukhwinder33445
force-pushed
the
support-php-85
branch
from
d67ed2a to
dd095bd
Compare
|
I have updated the commit messages and descriptions as your suggestion. I will also correct the commits in other PRs. |
Changes made:
PHP 8.4:
Other changes:
scanDirectorieswas adjusted so that tests are always run with the dependencies installed bycomposer install.No changes are required to support PHP 8.5.
resolves #60