Sylius / SyliusResourceBundle

Simpler CRUD for Symfony applications
https://sylius.com
MIT License
219 stars 155 forks source link

ResourceLoader: is `criteria` relevant to all routes? #275

Open rimas-kudelis opened 3 years ago

rimas-kudelis commented 3 years ago

The following block in ResourceLoader applies criteria to all generated routes: https://github.com/Sylius/SyliusResourceBundle/blob/e2682a010c666b40ecbc0aa2a8dbf74b69bf1abb/src/Bundle/Routing/ResourceLoader.php#L154-L156

I'm wondering if this is indeed useful. The problem I'm facing is that I want to use a URL parameter in an expression in the criteria to contatenate it with another variable I'm getting from the container, like this:

code: 'expr:service("security.token_storage").getToken().getUser().getVendor().getCode() ~ "_" ~ $unprefixedCode'

This works just fine for the show route, but causes the following error when accessing the index route:

Unexpected token "end of expression" of value "" around position 92 for expression service("security.token_storage").getToken().getUser().getVendor().getCode() ~ "_" ~.

As you can see, the missing variable is not passed as an empty string or null, but is pretty much erased from the parsed expresssion. I've worked this issue around by decorating the sylius.routing.loader.resource service with a copy of this class, which only applies criteria to show, update, and delete routes, but I'm thinking that perhaps doing it this way could be useful for upstream as well.

vvasiloi commented 3 years ago

I'm sure the criteria can be useful for index routes for basic filtering of a collection, but when using the resource loader it's usually meant for single resource routes. However, I can see it used for all routes to filter out soft-deleted/archived/disable resources. Considering that this issue can be easily overcome by using the except/only options and separating the routes with different criteria if necessary, I'm not sure it's worth to change the current behavior and limit this option to certain routes.

rimas-kudelis commented 3 years ago

Shouldn't these soft-deleted records be excluded automatically, without specifying criteria?

vvasiloi commented 3 years ago

It was just an example, it could be a group filter, like a channel.

rimas-kudelis commented 3 years ago

Okay, fair enough.

But it appears to me that the specified criteria is already pretty much ignored for index requests. The only issue is that an attempt is made to parse it. I've added a following line to the defaults: unprefixedCode: '' And now, my index works same as with my class override: I'm getting a list of that vendor's products. As if criteria was not there at all.

vvasiloi commented 3 years ago

It's used with filterable: true. https://github.com/Sylius/SyliusResourceBundle/blob/e2682a010c666b40ecbc0aa2a8dbf74b69bf1abb/src/Bundle/Controller/ResourcesResolver.php#L38-L40

rimas-kudelis commented 3 years ago

Ah! Ok, thanks, you're helpful as always! :) Do you work for Sylius by any chance? :)

rimas-kudelis commented 3 years ago

Then again, the condition could still be changed like this:

        if (!empty($configuration['criteria'])
            && ($configuration['filterable'] ?? false || in_array($actionName, ['show', 'update', 'delete']))) {
            $defaults['_sylius']['criteria'] = $configuration['criteria'];
        }

Although it's less of an issue ever since it dawned on me that I can just supply a blank default value.

vvasiloi commented 3 years ago

No, I don't.