Migrate part of the tools namespace to PHP8 syntax #9900
Conversation
| * {@inheritdoc} | ||
| * | ||
| * @return int | ||
| */ | ||
| #[ReturnTypeWillChange] |
There was a problem hiding this comment.
The attribute can be removed now.
There was a problem hiding this comment.
I have to look at the files one by one because for instance here, the constructor was not affected by Rector.
greg0ire
force-pushed
the
php8-migration
branch
3 times, most recently
from
c0b915c to
1f46cda
Compare
|
I think it will be better to touch the walkers after #9551 is finished. I will remove change to them from this PR |
greg0ire
changed the title
| * @psalm-return ArrayIterator<array-key, T> | ||
| */ | ||
| #[ReturnTypeWillChange] | ||
| public function getIterator() | ||
| public function getIterator(): ArrayIterator |
There was a problem hiding this comment.
It was already an ArrayIterator before, but should this method rely on an Iterator implementation in the return type instead of the interface itself? Even the name of the method doesn't refer to a specific Iterator implementation.
It's not really a blocker for a merge, but food for thoughts for 3.0.
There was a problem hiding this comment.
No, no, I totally see what you mean: if the consumer don't leverage it, why should we provide that level of detail. Here, they shouldn't be calling getIterator() directly but use a foreach, so that's one more reason to do this. Let me change that.
There was a problem hiding this comment.
Yes, IteratorAggregate allows to access getIterator() directly. Technically it should be Traversable as the docs suggests, but I wasn't sure if this step was a too large for this PR.
That way we have some level of error handling, and don't muddy the return type.
No description provided.