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: Cleanup illegal dependency from Flow to Neos.Media #3272

Open
mhsdesign opened this issue Jan 21, 2024 · 7 comments · May be fixed by #3285
Open

TASK: Cleanup illegal dependency from Flow to Neos.Media #3272

mhsdesign opened this issue Jan 21, 2024 · 7 comments · May be fixed by #3285

Comments

@mhsdesign
Copy link
Member

mhsdesign commented Jan 21, 2024

The resource:clean command is intertwined with Neos.Media's AssetRepository and AssetRepository.
This is hard to type for phpstan and generally unpleasant to look at :D

Flow 9 should provide some kind of extensibility so that Neos.Media can hook in and do its job.

// FIXME flow has no dependency on Neos.Media. This code should be extracted. https://github.com/neos/flow-development-collection/issues/3272

mhsdesign added a commit that referenced this issue Jan 21, 2024
…ation

This must be cleaned up at some point:
See #3272
mhsdesign added a commit that referenced this issue Jan 21, 2024
…ation

This must be cleaned up at some point:
See #3272
@mhsdesign
Copy link
Member Author

But that doesnt turn out to be thaaat straight forward, at least not with ugly and hard to understand hooks/interfaces.

The functionality is as follows:

If the Neos.Media package is active, this command will also detect any assets referring to broken resources and will remove the respective Asset object from the database when the broken resource is removed.

This part is very transparent, also the related assets will be shown when one is asked to delete a persistent resource, and after a deletion of a single resource it is shown how many assest were also purged:

Removed %s asset object(s) from the database.

A similar hook is in place for the thumbnails, but its a little less obvious, as its not documented in the command and only a deletion with related thumbnails will show:

Removed %s thumbnail object(s) from the database.

Im not sure what the best solution to approach this is, and if we can leverage doctrine lifetime cycle's or the like ...

mhsdesign added a commit that referenced this issue Jan 24, 2024
…ation

This must be cleaned up at some point:
See #3272
@kitsunet
Copy link
Member

Can't we simply introduce a signal ala PersistentResourceNoLongerExists with the object? then an implementation in the media package can take care of handling anything on that signal

@mhsdesign
Copy link
Member Author

yes sure, buuut we wont be able to log it to the console no?

image

@kitsunet
Copy link
Member

kitsunet commented Jan 25, 2024

There is ways I guess, but nothing too beautiful indeed. We could pass the output to the signal but that would bind us to CLI there, or equally dirty but a bit more standalone-ish, we pass somehting like a Messages object and the slot code just writes into that and we read the messages after the signal

$messages = new Messages();
$this->emitPersistentResourceNotLongerExists($persistentResource, $messages);
$this->output($messages->toString());

pseudo code but you get my drift....

Alternatively, what if we defined an interface for "plugins" for this, that defines a method that gets a resource and can return a message of some shape and don't define a signal but just go over all implementations of this interface and call the method.

@mhsdesign
Copy link
Member Author

mhsdesign commented Jan 25, 2024

Hmm yes sure, but to 100% reflect the current state we need a "beforePossibleRemove" hook as well, to display the related assets that would possibly be removed.

I think that we should rather have a specialised command sitting in neos.media and doing this job?

I fear all those signals will make the code less easy to grasp. Now its at least all in one place.

@kitsunet
Copy link
Member

separate command is perfeclty fine for me

@mhsdesign
Copy link
Member Author

mhsdesign commented Jan 25, 2024

so

flow resource:clean

will only remove the persistent resources, but ignore all assets, that still might be attached to them.

And

flow media:cleanresources (inspired by media:importresources) will remove the persistent resources AND keeping assets in mind.?

So using the original flow resource:clean would of course cause a little confusion in the migration period, we should probably warn them like this:

flow's resource:clean is only safe to be used in a pure flow installation without Neos.Media. To also cleanup assets you must use media:cleanresources. If you still want to continue and only want to cleanup resources press Y.

Aaand additionally the media cleanup could also cleanup orphaned assets, if the resource was removed via flow's resource:clean.

neos-bot pushed a commit to neos/flow that referenced this issue Jan 25, 2024
mhsdesign added a commit to mhsdesign/flow-development-collection that referenced this issue Jan 27, 2024
Resolves neos#3272

There will be another PR towards Neos.Media to include this functionality in `./flow media:cleanresources`.

Additionally warn if this command is used in a Neos installation, and let the dev know that it's preferred to use Neos' command.
mhsdesign added a commit to mhsdesign/flow-development-collection that referenced this issue Jan 27, 2024
Resolves neos#3272

There will be another PR towards Neos.Media to include this functionality in `./flow media:cleanresources`.

Additionally warn if this command is used in a Neos installation, and let the dev know that it's preferred to use Neos' command.
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 a pull request may close this issue.

2 participants