neos-development-collection icon indicating copy to clipboard operation
neos-development-collection copied to clipboard

BUGFIX: Show asset count for assets from readonly asset sources

Open bwaidelich opened this issue 1 year ago • 11 comments

For asset sources that have isReadOnly() set, instead of the edit action, the show action is resolved by default. But the corresponding template is missing the "usages" button

Fixes: #3989

bwaidelich avatar Aug 10 '23 18:08 bwaidelich

But one thing i still dont understand. I tested this out with the exact same change as you have in your PR. And in a asset source for example: https://github.com/daniellienert/assetsource-unsplash the asset count was still not showing up 🤔

crydotsnake avatar Aug 10 '23 18:08 crydotsnake

Did you maybe just reload the show action instead of going back to the list view and then click the image (to end up in the edit action) after applying the fix?

bwaidelich avatar Aug 10 '23 19:08 bwaidelich

instead of going back to the list view and then click the image (to end up in the edit action) after applying the fix?

I'm sure i did exactly this..

crydotsnake avatar Aug 11 '23 13:08 crydotsnake

@crydotsnake Just to confirm that we don't talk cross purpose: With

I'm sure i did exactly this..

You mean, that you applied the fix, navigated back to the list, then detail view and could not reproduce the fix?

bwaidelich avatar Aug 11 '23 14:08 bwaidelich

Actually there is no asset property in those external asset sources I checked (including @crydotsnake example :wink:). And Neos\Media\Domain\Model\AssetSource\AssetProxy\AssetProxyInterface also doesn't indicate that this would be a given.

As far as I can see {assetProxy.asset} will only return an object for Neos\Media\Domain\Model\AssetSource\Neos\NeosAssetProxy.

ahaeslich avatar Aug 11 '23 15:08 ahaeslich

Actually there is no asset property in those external asset sources I checked

Ah right, good point. The custom implementations have to implement that if they want to support this feature, i.e.

final class SomeAssetProxy implements AssetProxyInterface, HasRemoteOriginalInterface, SupportsIptcMetadataInterface
{
    /**
     * @Flow\Inject
     * @var AssetRepository
     */
    protected $assetRepository;
    
    /**
     * Return the locally imported asset (if any)
     * This is used in order to display the asset usages in the Neos.Media.Browser backend module
     *
     * @return AssetInterface|null
     */
    public function getAsset(): ?AssetInterface
    {
        $localAssetId = $this->getLocalAssetIdentifier();
        if ($localAssetId === null) {
            return null;
        }
        /** @var AssetInterface|null $asset */
        $asset = $this->assetRepository->findByIdentifier($localAssetId);
        return $asset;
    }
    
    // ...

not beautiful at all.. but this way the limitation can be worked around

bwaidelich avatar Aug 11 '23 15:08 bwaidelich

Thanks @ahaeslich for the clarification! So i assume this has to be fixed in the package itself :)

crydotsnake avatar Aug 11 '23 20:08 crydotsnake

What would be the best way too fix this in other asset source packages?

crydotsnake avatar Aug 11 '23 20:08 crydotsnake

Ping

crydotsnake avatar Aug 31 '23 15:08 crydotsnake

What would be the best way too fix this in other asset source packages?

I don't know.. :-/

I think inUse is evaluated on every call

I'm not sure I understand what you mean by "on every call". This PR just modifies the Show template, i.e. the display of a single asset

bwaidelich avatar Sep 27 '23 07:09 bwaidelich

You added the line <f:if condition="{assetProxy.asset.inUse}"> which will call all AssetUsageStrategies.

Sebobo avatar Sep 27 '23 07:09 Sebobo

I don't know how to proceed with this one.. @Sebobo you blocked this due to performance concerns. That's a fair point of course, but I don't see a different easy way to solve #3989 – do you have a suggestion?

bwaidelich avatar Apr 26 '24 14:04 bwaidelich

I would show the button without checking whether there are actual usages. In a medium sized project it can take up to a minute to calculate the usage. This would be super annoying when one wants to simply view the assets details.

Sebobo avatar Apr 26 '24 15:04 Sebobo

@Sebobo The button shows the usage count. You suggest to change that?

bwaidelich avatar Apr 26 '24 15:04 bwaidelich

@Sebobo just to make sure that we're on the same track: Currently, the button shows the usage count like: image

Besides, it is not rendered when there are no usages. We could change that to always show the button with a more generic label like "show usages" in any case and then resolve usages only if its clicked.

But: We currently disable the Delete Button in the footer if the asset is used, so this would have to be changed too..

In a medium sized project it can take up to a minute to calculate the usage

That's really annoying (and we should fix that by integrating something like Wwwision.AssetUsage to the core). But I don't see how that is related to this fix.

So could you please clarify what you suggest me to do to get a +1 (or at least no -1)?

bwaidelich avatar Apr 27 '24 13:04 bwaidelich

I get back to you on Monday. Have to check the code and Pr again from my computer instead of my phone 🤪

Sebobo avatar Apr 27 '24 17:04 Sebobo

OK now I know again. So the edit action already had this behaviour and had the annoying performance problem with assets that had more than a few usages.

With this change the show action which is also used by the UI editor would then have the same problem. I think at least the show action should remain lightweight and fast and not get slower now. Also this information is not as important there than it is in the editing mode.

I wonder whether we can extend the condition to fulfil both needs 🤔

Sebobo avatar Apr 29 '24 08:04 Sebobo

I'm not 100% sure, but IIRC we display the "show" template for readonly asset sources and the "edit" template for the others. As a result we don't display the usage count for readonly asset sources – which is a really bad limitation.

I need to dig into this once again – but maybe that is already fixed with the new media UI?

bwaidelich avatar Apr 29 '24 09:04 bwaidelich

Yes the new Media UI handles this by showing the usage count directly on the button if a fast strategy is available and showing them only on demand after the button is pressed if not. Similar behaviour with the delete button.

Sebobo avatar Apr 29 '24 09:04 Sebobo