Adding types#20
Conversation
gsteel
left a comment
gsteel
left a comment
There was a problem hiding this comment.
Adding native parameter and return types to public or protected members is a BC break - it can't be done in a minor unfortunately. Thumbs up on the changes to implicitly nullable types i.e. DomDocument $foo = null to DomDocument|null $foo = null as this will kill off deprecations in 8.4 as I'm sure you're aware.
| * @throws Exception\RuntimeException If entity expansion or external entity declaration was discovered. | ||
| */ | ||
| protected static function heuristicScan($xml) | ||
| protected static function heuristicScan(string $xml): void |
There was a problem hiding this comment.
That's a BC break on a non-final class. Feel free to add /** @final **/ to the class doc block to signal future intent, but this still can't be changed in a minor release.
There was a problem hiding this comment.
Also, you may want to add the BC Checker to CI like this: https://github.com/laminas/laminas-filter/pull/135/files
| private static function scanString( | ||
| string $xml, | ||
| DOMDocument|null $dom = null, | ||
| int $libXmlConstants, | ||
| callable $callback | ||
| ): SimpleXMLElement|DomDocument|bool { |
There was a problem hiding this comment.
@gsteel Since there are breaking changes anyway, should we use that opportunity to change the parameter order as well? For example this method has the optional parameter as second instead of last item.
There was a problem hiding this comment.
laminas-xml is in security-only mode so it's unlikely that we'll be releasing a new major version. I'm not 100% sure on the rules around this so I'll have to check with the @laminas/technical-steering-committee …
There was a problem hiding this comment.
Since there are breaking changes anyway…
Please note, this package is in security-only maintenance mode. See:
- https://github.com/laminas/laminas-xml#laminas-xml
- https://getlaminas.org/packages-maintenance-status/
This means that there will be no further updates or changes. Adding version 8.4 of PHP is the only bigger change, aside from security updates.
Some more background informations: https://getlaminas.org/blog/2023-11-28-laminas-and-mezzio-supports-php-83.html
Updated with php 8.4
alexmerlin
left a comment
alexmerlin
left a comment
There was a problem hiding this comment.
@SuperDJ Let's go through all the feedback received by this PR and wrap things up.
| "ext-dom": "*", | ||
| "ext-simplexml": "*" | ||
| }, | ||
| "ext-libxml": "*", |
There was a problem hiding this comment.
You should also fix indentation on these two lines.
4 participants
Description