Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

TYPO3 13 compatibility #1917

Open
wants to merge 4 commits into
base: development
Choose a base branch
from

Conversation

thomasrawiel
Copy link

@thomasrawiel thomasrawiel commented Oct 14, 2024

Here's my attempt for (beginning) TYPO3 13 compatibility

  • added version check for addRootlineFields (obsolete in v13)
  • Added a TSFE Utitlity class with version checks to try to substitute references to $GLOBALS['TSFE'] (Most TSFE members marked internal or read-only) while keeping compatiblity with old TYPO3 versions

related to #1901

@NamelessCoder
Copy link
Member

I appreciate the effort but unfortunately there's a lot of issues here - this list isn't exhaustive but if you want to have this merged it's a good place to start:

  • Most importantly: do everything you can to avoid "blanket" pull requests which change multiple things at the same time. Every commit should be possible to read with a lead-in "When applied, this patch will..." e.g. "When applied, this patch will add version check before defining addRootLineFields". And: smaller is better, incremental PRs that address the same issue in different places are very welcome!
  • Avoid making changes to your fork's development branch, use a feature branch. This is mostly for your own benefit as branches are almost always squashed before merged, causing your fork to fall out of sync and require re-checkout of the development branch.

Some basic dos-and-don'ts about code structure:

  • Never use new inside active code unless the object is perfectly "dumb", meaning that it calls no other third party code (basically: don't use new unless the object is a simple DTO or Enum-style container and even then, always consider a potential need for third parties to replace the implementation. Constructor arguments or GeneralUtility::makeInstance arguments is almost always the best approach). Objects instanced with new cannot be mocked in tests - something that is absolutely essential, especially when objects can do SQL requests. They also cannot be substituted by third parties registering different implementations through TYPO3 or DI.
  • "Utility" classes should only be suffixed "Utility" if they contain static methods, and static methods should never dispatch SQL queries for the reason above. The new utility added by this patch shouldn't be a utility, it should be a proxy/compatibility object.
  • Dependency instances should not be created in constructors unless this is necessary for legacy compatibility reasons (there are very few cases where this is the only way). Preferred method is constructor arguments (and if necessary, entries in Services.yaml to declare the object that uses the dependency as public).
  • Unit test, code style and PHPStan pass is mandatory for any PR to this project. You can run those via ./vendor/bin/phpunit, ./vendor/bin/phpstan and ./vendor/bin/phpcs. I'm almost certain none of these tests will pass for this PR just based on reading the source.

More general about the specific implementations:

  • The two methods added to CoreUtility are redundant. Use version_compare and VersionNumberUtility::getCurrentTypo3Version to do version conditions (find examples around the existing source code).
  • Avoid redundant phpdoc (only use them if you need to convey information that isn't specified by strict types + nullable, for example if there is no strict type annotation).
  • Don't use union types (you didn't, I'm noting it here juts in case). Dependencies currently must allow PHP versions that do not support union types so we have to avoid them for now.
  • Do not create dependency instances inline in code; if a subject needs a dependency (that is not called statically), constructor argument injection is preferred.
  • This isn't visible from the code base so it's not your fault at all, but: CoreUtility and a couple of other ones only exist for legacy reasons - they were made for cases where different methods needed to be called in different ways on different TYPO3 versions, but since dropping support for TYPO3 <10.4 they are no longer necessary and will be removed. So as far as possible, steer clear of adding new methods on those utilities.

My first advise is to split this up into smaller parts and make sure each one passes the unit/phpstan/phpcs tests when executed with a TYPO3v13 set of dependencies. Work is currently in progress to add these test steps to the CI but for now they can only be run manually; after temporarily requiring updated dependencies in the composer.json manifest (which then shouldn't be committed).

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

Successfully merging this pull request may close these issues.

2 participants