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

[TASK] Add new page repository alias #1738

Conversation

tobsnti
Copy link

@tobsnti tobsnti commented Oct 10, 2021

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

Because PageRepository has been moved with TYPO3 11 the class alias is created based on class existence.
@NamelessCoder
Copy link
Member

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
Copy link
Author

tobsnti commented Oct 12, 2021

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

Use class alias loader to create composer autoload entry's for deprecated classes.
@NamelessCoder
Copy link
Member

@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
Copy link
Author

tobsnti commented Oct 21, 2021

@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
Copy link
Member

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
Copy link

helsner commented Dec 23, 2021

I guess this will not properly work in the long run, at least IMHO
(

$pageRepository->init(false);
)
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

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.

3 participants