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: Separate Booting\Scripts and CLI subrequest calls #3372

Open
wants to merge 1 commit into
base: 9.0
Choose a base branch
from

Conversation

kitsunet
Copy link
Member

This introduces Neos\Flow\Core\PhpCliCommandHandler to run CLI subrequests instead of using \Neos\Flow\Core\Booting\Scripts as that has grown way to big over the years and this single responsibility can be cleanly separated into a different class.

Some minimal cleanup was done along the way.

The old @api methods still exist in Scripts as deprecated shims so this should not be breaking for userland code.

This introduces `Neos\Flow\Core\PhpCliCommandHandler` to run
CLI subrequests instead of using `\Neos\Flow\Core\Booting\Scripts` as
that has grown way to big over the years and this single responsibility
can be cleanly separated into a different class.

Some minimal cleanup was done along the way.

The old `@api` methods still exist in Scripts as deprecated shims
so this should not be breaking for userland code.
@kitsunet
Copy link
Member Author

I am not quite sure what phpstan is telling me, but yeah, I think this makes it prettier...

@kitsunet kitsunet requested a review from mhsdesign June 23, 2024 19:39
@mhsdesign
Copy link
Member

Nice :D Looks already better :D Have you read my thoughts over at #3112 ?

Like i agree that the scripts utility is ugly but when we introduce a new api we should be sure it can last another 10 years or so :D
For example, if we prefer to have rather a instance instead of static utility, the time would be now (the static $builtPhpCommand cache introduced with #3119 was not meant to live that way forever, and i hoped to be able to cleanup this mess at some point :D)
Also considerations to adapt symfony/process should be brought up now or never again :D
(we have it installed already ... so that would not be the point :D)

composer/composer 2.6.6   requires  symfony/process (^5.4 || ^6.0 || ^7)          
symfony/console   v6.4.3  conflicts symfony/process (<5.4) 

@kitsunet
Copy link
Member Author

Yeah, it's really tricky, I am not fond of the static thing, and I guess we could make this an instance but then the cache would be totally meaningless (unless we create this super early as singleton scope and the bootstrap/objectmanager carries it around). I think the cache is a good idea because figuring all this out is expensive, in an ideal world we would even compile that into a class once for production because it shouldn't change for that same system. Anyways I am not super sure how well it works as "you have to inject this for subrequest", but then it shouldn't be a mainstream usecase anyways? So I guess going down that route would at least give us options for later, because then we can still refactor it internally to process if we want to....

@mhsdesign
Copy link
Member

I think compiling this builtPhpCommand once into a cache or php class is only kindof a good idea. Currently it does different checks if triggered from cli or web and these integrity checks do seem like a good idea to avoid absolute chaos :D
Carrying it around as singleton might be indeed cleaner? I mean flow was always about di and objects ... never so much (luckily) about dirty static utilities. Using symfony would provide fancy apis on top but we probably wouldnt want to expose that? And if we dont we dont need it 😂 ... i figured out that our async thing for example is more reliable than the symphony thing as this will just straight die if the main process shuts down.
But lets chat about this soon?

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

Successfully merging this pull request may close these issues.

2 participants