Use PSR-4 namespaced classes

[Art4]

Hey 👋

Small note about myself: English is not my native language, so I apologize for any language mistakes. I am a long time user of SimplePie directly or indirectly (through WordPress or other libraries). I would like to give something back to the community.

I took some time over the weekend to refactor SimplePie to use namespaces. This means that this PR is not yet ready for merging, but relies on merging #711 and still needs some work and a lot of discussion.

Update: This PR is nearly ready for merge!

task list

• Merge Add namespaced classes and PSR-4 support #711 for having pseudo-classes with namespace in the src folder and aliases created in the library folder.
• move the code from the library classes to the namespaced classes
• internally only use the new namespaced classes
• inside the src folder don't use any code from the library folder
• create aliases in the src folder, e.g. class_alias('SimplePie\Cache', 'SimplePie_Cache');
• leave only pseudo-classes or deprecated code in library folder
• move all global SIMPLEPIE_* constants into the SimplePie\SimplePie class
• set deprecate comment for all global SIMPLEPIE_* constants
• update autoloader.php
• update demo/ folder
• update build/compile.php
• update README.md
• be more specific in the deprecation notices in library classes
• update CHANGELOG.md

Follow up changes

All the changes in this PR did not change any tests, so all tests (except the new tests from #711) run with the old non-namespaced classes. I intentionally did not change the old tests to show that the old code continues to work as before. These tests need to be modified (or better yet, first copied and then modified) to test the namespaced classes directly. Update: See #730

Possible roadmap

Since I couldn't find any information about the future development of SimplePie or a roadmap, I've sketched out a possible roadmap of how these changes could be released incrementally. If I missed an existing roadmap, I would be happy to be pointed to the right place.

Update: I moved the Roadmap into a separate issue: #731

More details

1. Why should #711 be released in a minor release?

Following semantic versioning we have three options:

• The new namespaced classes are keeping the backward compatibility. A new MAJOR version (e.g. v2.0.0) would be need if we make incompatible API changes. ❌
• The new namespaced classes aren't a bugfix so a PATCH version (e.g. v1.5.9) is also not recommended. ❌
• The new namespaced classes are a new feature, because they allow the usage of PSR-4. Adding functionality in a backwards compatible manner should be done in a MINOR version (e.g. v1.6.0) ✅

2. Why did you move the SIMPLEPIE_* constants into the SimplePie\SimplePie class?

The SIMPLEPIE_* constants are defined in file library/SimplePie.php next to the class definition, but no code in the src folder should use code from the old library folder. Sure, we have the option to move the constant definitions in another file inside the src folder. But this would also lead to two downsides.

1. Because a global constant could not be defined in two places we would have to check about the existence of the constant before we could define it. And because we don't know, if the user will use the old library/SimplePie.php or the new src/SimplePie.php file (so we don't know, which file would be load first) we have to keep and maintain this code in two places:

// in library/SimplePie.php
// and in src/SimplePie.php
- define('SIMPLEPIE_NAME', 'SimplePie');
+ if(! defined('SIMPLEPIE_NAME'))
+ {
+     define('SIMPLEPIE_NAME', 'SimplePie');
+ }

2. Users could very easily overwrite the constants intentionally or unintentionally. This is not possible so far and should not be possible so easily.

By moving the constants to the SimplePie\SimplePie class, I was able to work around both issues. At the same time, we keep the global namespace clean of our constants, most of which are only used internally anyway.

However, two questions remain that should be discussed:

1. Is the naming of the constants ok (e.g. SimplePie\SimplePie::SIMPLEPIE_NAME), or should we rename them (e.g. SimplePie\SimplePie::NAME)?
2. Should all constants be defined in SimplePie\SimplePie or should we split them into different classes, where they will be primary used or even in a special constant class (e.g. \SimplePie\Constants::NAMESPACE_RSS_090 instead of \SimplePie\SimplePie::SIMPLEPIE_NAMESPACE_RSS_090)?

Trivia

When submitting a pull request

1. Run the linter.

   make lint

I've installed temporary PHPStan and analyzed the code in folders src and library with level 0 and fixed all errors and significant warnings.

2. Run the tests.

   make test

I've run bin/phpunit and all tests succeeded.

1. 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.

Please see the single commits for better understanding of the refactoring.

• Can your code review be broken into smaller chunks?

Yes, this PR could definitely be done in many smaller chunks over a bigger time period. But I wanted to show how the result at the end will look like. I'm fully aware that this PR is possibly to big and could be closed or has to be reworked.

• Make sure you have PHPUnit tests that cover your changes.

✅

4. 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.

✅

5. 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 [ ].

✅

6. 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.

I would be happy to do this. 💙 Let's talk and discuss!

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

✅

[Art4]

This PR is ready for merging.

It is important for me to emphasize that all changes are backward compatible! If you notice something that leads to a BC break, please let me know.

Nevertheless, I would like to hear feedback about some design decisions:

1. All globally defined constants are marked as deprecated.

I moved the global constants (like SIMPLEPIE_VERSION) to the SimplePie\SimplePie class. For easier migration I did not rename them.

The SIMPLEPIE_* constants are defined in file library/SimplePie.php next to the class definition, but no code in the src folder should use code from the old library folder. Sure, we have the option to move the constant definitions in another file inside the src folder. But this would also lead to two downsides.

1. Because a global constant could not be defined in two places we would have to check about the existence of the constant before we could define it. And because we don't know, if the user will use the old library/SimplePie.php or the new src/SimplePie.php file (so we don't know, which file would be load first) we have to keep and maintain this code in two places:

// in library/SimplePie.php
// and in src/SimplePie.php
- define('SIMPLEPIE_NAME', 'SimplePie');
+ if(! defined('SIMPLEPIE_NAME'))
+ {
+     define('SIMPLEPIE_NAME', 'SimplePie');
+ }

2. Users could very easily overwrite the constants intentionally or unintentionally. This is not possible so far and should not be possible so easily.

By moving the constants to the SimplePie\SimplePie class, I was able to work around both issues. At the same time, we keep the global namespace clean of our constants, most of which are only used internally anyway.

Is the naming of the constants ok (e.g. SimplePie\SimplePie::SIMPLEPIE_NAME), or should we rename them (e.g. SimplePie\SimplePie::NAME)?

Should all constants be defined in SimplePie\SimplePie or should we split them into different classes, where they will be primary used or even in a special constant class (e.g. \SimplePie\Constants::NAMESPACE_RSS_090 instead of \SimplePie\SimplePie::SIMPLEPIE_NAMESPACE_RSS_090)?

2. The constant SIMPLEPIE_BUILD is deprecated, use SimplePie\Misc::get_build() instead

SIMPLEPIE_BUILD relies at SimplePie\Misc::get_build() so it could not be called as definition of of class constant. So I deprecated it completely in favor of the direct use of SimplePie\Misc::get_build().

What do you think?

3. The constant SIMPLEPIE_USERAGENT is deprecated, use SimplePie\Misc::get_default_useragent() instead

SIMPLEPIE_USERAGENT relies at SIMPLEPIE_BUILD and has therefore the same problem as above. So I deprecated the constant in favor of the new method SimplePie\Misc::get_default_useragent().

What do you think?

[Art4]

Hey @mblaney, could you please review this PR or give me feedback on what you think of the PR? If it helps, I can still try to split the PR into smaller PRs, but really the individual commits should give you the ability to track the changes. I wrote down my plan for the roadmap in #731.

I have a high interest in modernizing SimplePie a bit, but I can't do that without feedback from the SimplePie team.
I'm sure other people in the community can be found to help modernize it. But for that we need a bit shorter response time from the SimplePie team.

[mblaney]

hi @Art4, sorry for the delay in reviewing this PR. I think removing SIMPLEPIE_BUILD and SIMPLEPIE_USERAGENT is ok, I like the idea of shortening the other constants too. I only have one question about this change, why couldn't the old Misc class be extended the same way as the other classes?

[Art4]

I think removing SIMPLEPIE_BUILD and SIMPLEPIE_USERAGENT is ok, I like the idea of shortening the other constants too.

Thank you for your feedback. I will shorten the prefix SIMPLEPIE_ in all constants (eg. SimplePie\SimplePie::NAME instead of SimplePie\SimplePie::SIMPLEPIE_NAME

I only have one question about this change, why couldn't the old Misc class be extended the same way as the other classes?

Good question. I missed that point in my comment about the design decisions, so I will add this answer.

The class SimplePie_Misc has the deprecated methods SimplePie_Misc::get_element() and SimplePie_Misc::entities_decode(). This methods were set deprecated in 2012 (see 878c83c).
This methods are not used internally and they are build on top of the also deprecated class SimplePie_Decode_HTML_Entities. As this class is deprecated it has not received an alias (see #711 (comment)).

So to keep BC for this deprecated methods we would have to create a namespaced class for the deprecated SimplePie_Decode_HTML_Entities. But after 11 years I don't think this methods are widly used today, so this methods don't exist in the SimplePie\Misc class.

This is not a breaking change because using SimplePie_Misc::get_element() and SimplePie_Misc::entities_decode() still work, but switching to SimplePie\Misc::get_element() or SimplePie\Misc::entities_decode() would require manual changes but will not work. I have to revise my answer. Since SimplePie 1.6.0 the use of SimplePie\Misc::get_element() and SimplePie\Misc::entities_decode() is possible, therefore this change would now be a breaking change. I need to find another solution for this.

I will reflect this changes in the CHANGELOG.md.

[Art4]

@mblaney I renamed all SimplePie\SimplePie constants and fixed the BC breaks in SimplePie\Misc. The SimplePie_Misc class is now extended the same way as the other classes. The only drawback is that we now call the deprecated \SimplePie_Decode_HTML_Entities class which is located in library/, so I missed on of my goals:

[ ] inside the src folder don't use any code from the library folder

But I think this way is better than creating a new deprecated class \SimplePie\Decode\HTML\Entities. What do you think?

[mblaney]

thanks @Art4 these changes look great! I agree that calling \SimplePie_Decode_HTML_Entities seems like the best way to go for now. Well done for making this happen, are you happy if I merge now?

[Art4]

From my side everything is ready but after merge the PRs #728 and #729 would have merge conflicts.

So I would therefore suggest that you decide on these two PRs beforehand and possibly merge them. Then I solve the merge conflict in here and the PR would be ready for merge.

[Art4]

@mblaney I resolved the merge conflicts. This PR is ready for merge.