FluidTYPO3 / vhs

TYPO3 extension VHS: Fluid ViewHelpers
https://fluidtypo3.org
Other
190 stars 229 forks source link

[TASK] Add new page repository alias #1738

Closed tobsnti closed 2 years ago

tobsnti commented 2 years ago

Because PageRepository has been moved with TYPO3 11 the class alias is created based on class existence.

NamelessCoder commented 2 years ago

Although this does solve the problem, using class_alias() this way will pollute the class namespaces - I suggest a different solution that doesn't require aliasing, perhaps best solved by making a class that resolves the right instance based on TYPO3 version and then use that class everywhere that PageRepository is used. PageRepository probably won't be the last class that gets moved or renamed, so having a version-sensitive way of resolving such classes imho makes sense.

tobsnti commented 2 years ago

@NamelessCoder I've found this https://github.com/TYPO3/class-alias-loader in the core, would this be a solution that you accept? As I understand it, if no alias is found, there is no additional autoload entry, so we can asure full compatibility for old and new versions. If this solution suits you, I would test it with TYPO3 >8.

NamelessCoder commented 2 years ago

@tobsnti Does this also work for non-composer installs of TYPO3, starting from 8.7? If it does then I'm OK with this solution. We should then also make sure that the new class name is used throughout the code.

tobsnti commented 2 years ago

@NamelessCoder It's not working for non-composer installations. How about a function in the CoreUtility that resolves the needed class for the current core version and returns it. This way we could create a dynamic config file that can easily be extended for other cases.

The config could be something like that: return [ 'TYPO3\\CMS\\Frontend\\Page\\PageRepository' => [ 11 => \TYPO3\CMS\Core\Domain\Repository\PageRepository::class ], ]; Basically we filter for the FQCN and suitable core version, in this case version 11 and higher would return the new class for the PageRepository.

Another solution, which I would consider a bit dirty, would be a PHP file that is included based on the core version, which creates the removed FQCN with an extend on the current class.

<?php
namespace TYPO3\CMS\Frontend\Page {
    class PageRepository extends \TYPO3\CMS\Core\Domain\Repository\PageRepository
    {
    }
}
NamelessCoder commented 2 years ago

Sorry for the delay here, @tobsnti - I've been very busy elsewhere.

Re: this PR, I would very much prefer to have a Factory that gives us the right instance based on presence of one class or the other. Something like this (freestyle):

class InstanceFactory {
    /**
     * @return \TYPO3\CMS\Frontend\Page\PageRepository|\TYPO3\CMS\Core\Domain\Repository\PageRepository
     */
    public function getPageRepository()
    {
        if (class_exists(\TYPO3\CMS\Frontend\Page\PageRepository::class)) {
            return GeneralUtility::makeInstance(\TYPO3\CMS\Frontend\Page\PageRepository::class);
        }
        return GeneralUtility::makeInstance(\TYPO3\CMS\Core\Domain\Repository\PageRepository::class);
    }
}

And then use this factory in all places where we need a PageRepository.

Even though at first this InstanceFactory would only have a single method to get the PageRepository, I'm convinced it won't be the last time we run into a requirement of this type so imho it makes a lot of sense to have a factory like this, that would allow us to "hide away" the actual class name that's returned as well as any differences in things like required constructor arguments or details about how the instance is constructed (makeInstance or ObjectManager or DI container fetch, for example). In short, I'd prefer if we handle this with "real" code instead of using metadata files or aliasing. This also has the benefit of working the exact same way in composer/non-composer and unit test context. It requires more refactoring in places where we use the PageRepository but I think it's a better solution in the long run.

helsner commented 2 years ago

I guess this will not properly work in the long run, at least IMHO (https://github.com/FluidTYPO3/vhs/blob/04d7ac203d7726738b280ee2f2eec2ae24c73b48/Classes/ViewHelpers/Page/Resources/FalViewHelper.php#L78) Here ->init() is called which is protected in v11, thus not callable. So this would need a workaround as well. I will llok after the holidays