Skip to content
This repository has been archived by the owner on May 26, 2022. It is now read-only.

Deprecated warnings for incompatible return types in PHP 8.1 #846

Closed
fabacino opened this issue Jan 6, 2022 · 7 comments · May be fixed by #874
Closed

Deprecated warnings for incompatible return types in PHP 8.1 #846

fabacino opened this issue Jan 6, 2022 · 7 comments · May be fixed by #874

Comments

@fabacino
Copy link

fabacino commented Jan 6, 2022

Since PHP 8.1 most methods of internal classes require a compatible return type when overriding them (see Return Type Compatibility with Internal Classes).

This results in deprecated warnings for the following classes:

  • Box\Spout\Reader\CSV\RowIterator
  • Box\Spout\Reader\CSV\SheetIterator
  • Box\Spout\Reader\ODS\RowIterator
  • Box\Spout\Reader\ODS\SheetIterator
  • Box\Spout\Reader\Wrapper\XMLReader
  • Box\Spout\Reader\XLSX\RowIterator
  • Box\Spout\Reader\XLSX\SheetIterator

Sample output for Box\Spout\Reader\CSV\RowIterator:

❯ vendor/bin/phpunit | grep -i "deprecated: return type" | grep -i 'csv\\rowiterator'

Deprecated: Return type of Box\Spout\Reader\CSV\RowIterator::current() should either be compatible with Iterator::current(): mixed, or the #[\ReturnTypeWillChange] attribute should be used to temporarily suppress the notice in src/Spout/Reader/CSV/RowIterator.php on line 227
Deprecated: Return type of Box\Spout\Reader\CSV\RowIterator::next() should either be compatible with Iterator::next(): void, or the #[\ReturnTypeWillChange] attribute should be used to temporarily suppress the notice in src/Spout/Reader/CSV/RowIterator.php on line 129
Deprecated: Return type of Box\Spout\Reader\CSV\RowIterator::key() should either be compatible with Iterator::key(): mixed, or the #[\ReturnTypeWillChange] attribute should be used to temporarily suppress the notice in src/Spout/Reader/CSV/RowIterator.php on line 238
Deprecated: Return type of Box\Spout\Reader\CSV\RowIterator::valid() should either be compatible with Iterator::valid(): bool, or the #[\ReturnTypeWillChange] attribute should be used to temporarily suppress the notice in src/Spout/Reader/CSV/RowIterator.php on line 117
Deprecated: Return type of Box\Spout\Reader\CSV\RowIterator::rewind() should either be compatible with Iterator::rewind(): void, or the #[\ReturnTypeWillChange] attribute should be used to temporarily suppress the notice in src/Spout/Reader/CSV/RowIterator.php on line 87

Adding the #[\ReturnTypeWillChange] attribute to these methods fixes the issue. I can PR if appreciated.

@ankurk91
Copy link

ankurk91 commented Jan 7, 2022

Please send a PR, this is the only way to fix the issue

@adrilo
Copy link
Collaborator

adrilo commented Jan 8, 2022

Hi! Feel free to send a PR :)

Just one thing: if you take a look at the CSVIterator::rewind method for instance:

    /**
     * Rewind the Iterator to the first element
     * @see http://php.net/manual/en/iterator.rewind.php
     *
     * @return void
     */
    public function rewind()
    {
        ...
    }

As you can see, the PHPDoc is correct. To solve the issue you mentioned, the correct fix would be to add a proper return type (instead of adding the #[\ReturnTypeWillChange] attribute).

@fabacino
Copy link
Author

fabacino commented Jan 10, 2022

As you can see, the PHPDoc is correct. To solve the issue you mentioned, the correct fix would be to add a proper return type (instead of adding the #[\ReturnTypeWillChange] attribute).

Adding the return type would break backward compatibility and assuming that this fix would get released as a patch version, I am not sure this would be the best solution. If this is not a concern, however, I can certainly PR accordingly.

@adrilo
Copy link
Collaborator

adrilo commented Jan 11, 2022

Well most of the methods are (or should be considered) internal only. So adding a return type should not break anybody.
Also, the @return in the PHPDocs should be correct. So the only case where it's going to break something is if someone used an internal method that did not have the correct type.
Is that correct?

@fabacino
Copy link
Author

Well most of the methods are (or should be considered) internal only. So adding a return type should not break anybody.
Also, the @return in the PHPDocs should be correct. So the only case where it's going to break something is if someone used an internal method that did not have the correct type.
Is that correct?

Well, you could get a break even with a correct return type because adding explicit return types forces all extending classes to have explicit return types as well, otherwise PHP will raise a fatal error (function interface mismatch).
So if the RowIterator::rewind() function interface were changed to

class RowIterator implements IteratorInterface
{
    /**
     * Rewind the Iterator to the first element
     * @see http://php.net/manual/en/iterator.rewind.php
     */
    public function rewind(): void
    {
        ...
    }
}

then an overridden function with the current interface in an extending class

class MyIterator extends RowIterator
{
    /**
     * Rewind the Iterator to the first element
     * @see http://php.net/manual/en/iterator.rewind.php
     *
     * @return void
     */
    public function rewind()
    {
        ...
    }
}

would cause a break.

@fabacino
Copy link
Author

Please let me know whether you prefer the addition of explicit return types (despite possible breaks) or want it to be solved with attributes.

@adrilo
Copy link
Collaborator

adrilo commented Feb 12, 2022

@fabacino As part of another PR, I fixed these issues (using explicit types)

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants