[PoC] Resource operations #17695
[PoC] Resource operations #17695
Conversation
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
| sylius_resource: | ||
| mapping: | ||
| imports: | ||
| - "%kernel.project_dir%/src/Sylius/Bundle/AdminBundle/Resources/config/app/sylius/resources" |
There was a problem hiding this comment.
Of course, this needs to be fixed.
It should be @SyliusAdminBundle/Resources/config/app/sylius/resources
We should also configure this on another config file to provide a bc-layer.
Routes will still be built with ResourceController system by default on 2.x and should be removed on 3.x.
src/Sylius/Bundle/AdminBundle/Resources/config/app/sylius/resources/zone.php
Outdated
Show resolved
Hide resolved
src/Sylius/Bundle/AdminBundle/Resources/config/app/sylius/resources/inventory.php
Outdated
Show resolved
Hide resolved
b25573c to
a365750
Compare
src/Sylius/Bundle/AdminBundle/Resources/config/app/sylius/resources/payment.php
Outdated
Show resolved
Hide resolved
src/Sylius/Bundle/AdminBundle/Resources/config/app/sylius/resources/payment.php
Outdated
Show resolved
Hide resolved
6723998 to
d81a730
Compare
✅ Preview Environment deployed on Bunnyshell
See: Environment Details | Pipeline Logs Available commands:
|
d81a730 to
c742165
Compare
90988a1 to
7814307
Compare
7814307 to
694f23e
Compare
| use Sylius\Resource\Metadata\Operations; | ||
| use Sylius\Resource\Metadata\ResourceMetadata; | ||
|
|
||
| return (new ResourceMetadata()) |
There was a problem hiding this comment.
This one has a small issue on the browser title.
de977b6 to
1655d6b
Compare
c1d3124 to
89075d5
Compare
| try { | ||
| $this->indexPage->deleteResourceOnPage(['code' => $productVariant->getCode()]); | ||
| } catch (ResourceDeleteException $exception) { | ||
| $this->sharedStorage->set('last_exception', $exception); |
There was a problem hiding this comment.
The error message is still ok.
efb4fe7 to
5f37500
Compare
features/admin/order/managing_orders/handling_order_payment/finalizing_order_payment.feature
Outdated
Show resolved
Hide resolved
| @@ -34,6 +36,7 @@ public function __construct( | |||
| private readonly UpdatePageInterface $updatePage, | |||
| private readonly FormElementInterface $formElement, | |||
| private readonly NotificationCheckerInterface $notificationChecker, | |||
| private SharedStorageInterface $sharedStorage, | |||
There was a problem hiding this comment.
| private SharedStorageInterface $sharedStorage, | |
| private readonly SharedStorageInterface $sharedStorage, |
| use Sylius\Resource\Metadata\Update; | ||
|
|
||
| return (new ResourceMetadata()) | ||
| ->withRoutePrefix('/admin') |
There was a problem hiding this comment.
Use the parameter instead:
| ->withRoutePrefix('/admin') | |
| ->withRoutePrefix('/%sylius_admin.path_name%') |
There was a problem hiding this comment.
Same for every admin resource definition.
| ->withRoutePrefix('/admin') | ||
| ->withClass('%sylius.model.admin_user.class%') | ||
| ->withSection('admin') | ||
| ->withTemplatesDir('@SyliusAdmin\\shared\\crud') |
There was a problem hiding this comment.
I see this a lot but Symfony can handle both / and \ format.
To avoid using too many chars this can be simplified like this:
| ->withTemplatesDir('@SyliusAdmin\\shared\\crud') | |
| ->withTemplatesDir('@SyliusAdmin/shared/crud') |
Same for the rest of the resource definitions
src/Sylius/Bundle/AdminBundle/Resources/config/app/sylius/resources/admin_user.php
Show resolved
Hide resolved
| return (new ResourceMetadata()) | ||
| ->withRoutePrefix('/admin') | ||
| ->withClass('%sylius.model.admin_user.class%') | ||
| ->withSection('admin') |
There was a problem hiding this comment.
💡 This can be transform to a class constant or a parameter one day
There was a problem hiding this comment.
Could also somehow be joined w/ the SectionInterface objects. That'd probably necessitate moving the interface to ResourceBundle, but ultimately should ease resolving and basing some logic on specific sections.
Food for thought.
| formType: ProductGenerateVariantsType::class, | ||
| notificationMessage: 'sylius.product_variant.generate', | ||
| redirectToRoute: 'sylius_admin_product_variant_index', | ||
| redirectArguments: ['productId' => "request.attributes.get('productId')"], |
There was a problem hiding this comment.
There is a notation used to specify that the value has to be interpreted, why in this case we are not using :
| redirectArguments: ['productId' => "request.attributes.get('productId')"], | |
| redirectArguments: ['productId' => "@=request.attributes.get('productId')"], |
There was a problem hiding this comment.
currently here, we can only have an expression.
So, we need to
- allow this extra characters like you did here..
- deprecate using expresion without using "@="
- allow simple strings
| new Create(redirectToRoute: 'sylius_admin_taxon_update'), | ||
| new Create( | ||
| path: 'taxons/new/{id}', | ||
| template: '@SyliusAdmin/shared/crud/create.html.twig', |
There was a problem hiding this comment.
If the resource use : @SyliusAdmin/shared/crud as templateDir is this template required ?
There was a problem hiding this comment.
I need to check that one. I think it uses the short name of the operation.
| #sylius_admin_admin_user: | ||
| # resource: | | ||
| # alias: sylius.admin_user | ||
| # section: admin | ||
| # path: users | ||
| # templates: "@SyliusAdmin\\shared\\crud" | ||
| # except: ['show'] | ||
| # redirect: index | ||
| # grid: sylius_admin_admin_user | ||
| # form: | ||
| # type: Sylius\Bundle\AdminBundle\Form\Type\AdminUserType | ||
| # permission: true | ||
| # type: sylius.resource |
There was a problem hiding this comment.
Can we move all route resource to a dedicated file, so we can migrate resources one by one?
There was a problem hiding this comment.
I think we cannot move routing files. The file is public, so we can break things.
I need to test routing condition to bring a bc-layer with that kind of thing
https://github.com/monsieurbiz/SyliusCmsPagePlugin/blob/8cc79bf5e0a3b66799868ad4ff7e13086f80b347/src/Routing/RequestContext.php#L42
4eeac1b to
c75eba4
Compare
c75eba4 to
8fd5c47
Compare
Linked to Sylius/SyliusResourceBundle#983