Typehinting

[ColonelMoutarde]

When submitting a pull request

1. Run the linter.

   make lint

2. Run the tests.

   make test

3. Carefully read your diff before submitting a PR.

   • Make sure your diff clearly represents your changes.
   • Make sure your code is easy for reviewers to follow.
   • Can your code review be broken into smaller chunks?
   • Make sure you have PHPUnit tests that cover your changes.

4. Make sure that your changes are very-nearly complete.

   • Small changes/fixes based on feedback are OK.
   • Major changes should be avoided while a PR is open. If you need to make major changes, close the PR, make your changes, and open a new PR once you're ready.

5. In the description, start with an explanation of the change and why you want to make it.

   • Make any relevant documentation easily available for reviewers.

6. Add a list of changes you've made using task list syntax.

   • Things that are complete should have [X].
   • Things that are still incomplete should have [ ].

7. Discuss feedback.

   • The SimplePie NG Core Team has the final say on what gets accepted.
   • Respond to all code review feedback.
   • Respectfully discuss any feedback you disagree with. You may need to bring a better argument to the table to convince us.

8. If you don't understand something, ask.

[jtojnar]

This will be a BC break.

[Maikuolan]

You can ignore this reply (I wrote something wrong). :-)

(Unless you really want to read it. Not going to delete it because deleting one's own replies at GitHub is in bad taste IMO).

This will be a BC break.

^ That.

Specifically, support for return type declarations wasn't added until PHP 7.0, so adding them to SimplePie would, at the very least, necessitate increasing the minimum required PHP version as stated at the repository's README and composer.json files from 5.3 to 7.

See:

• https://www.php.net/manual/en/migration70.new-features.php
• https://wiki.php.net/rfc/return_types

(None of which necessarily makes it a bad idea. Declaring return types for all methods is a nice thing to do for modern, clean, well-written PHP code. However, if it were to be done, I would suggest earmarking it for a future new major version of SimplePie, rather than the current major version, in order to remain semver-compliant and to avoid surprising any users with the BC-break).

[jtojnar]

necessitate increasing the minimum required PHP version as stated at the repository's README and composer.json files from 5.3 to 7.

We are already targetting PHP 7.2 since SimplePie 1.8.0.

[Maikuolan]

We are already targetting PHP 7.2 since SimplePie 1.8.0.

Whoops.. So we are. Scratch my reply then. My bad. '^.^

Self-reminder: Looking at multiple different repositories/projects/etc at the same time isn't always such a great idea. SimplePie aside, there's another RSS library I was looking at which supported older PHP versions, and I think I got my wires crossed there.

Anyway, all good.

[ColonelMoutarde]

All fixed :°)

[ColonelMoutarde]

All tests success

[Art4]

I'm sorry, but as @jtojnar already said: This will be a BC break. Users who had extend e.g. Misc::fix_protocol() will get an exception.

[ColonelMoutarde]

I'm sorry, but as @jtojnar already said: This will be a BC break. Users who had extend e.g. Misc::fix_protocol() will get an exception.

Do you have an suggestion ?

[Art4]

Do you have an suggestion ?

Only suggestion is to defer your PR until we are working at release v2. Increasing the major version allows breaking changes.

But we are already planning to add return types by introducing new camelCase methods starting with 2.1.0, see #731.