Resource discovery: ignore comments/strings when detecting classes #1061
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Q | A | --------------- | ----- | Bug fix? | kind of | New feature? | yes | BC breaks? | no | Deprecations? | no | Related tickets | fixes #1032 | License | MIT The idea is to ensure we will not break userland application translations when removing dependency to a locale parameter.
Co-authored-by: Dmitri Perunov <[email protected]>
| Q | A | --------------- | ----- | Bug fix? | no | New feature? | no | BC breaks? | no | Deprecations? | no | Related tickets | partially #1029 | License | MIT
| Q | A | --------------- | ----- | Bug fix? | no | New feature? | yes | BC breaks? | no | Deprecations? | no | Related tickets | fixes #1029 | License | MIT Very similar to api-platform/core#7017 but in Sylius resource context (for HTML routing) ```php <?php // config/sylius/resources/speaker.php declare(strict_types=1); use Sylius\Resource\Metadata\Create; use Sylius\Resource\Metadata\ResourceMetadata; use Sylius\Resource\Metadata\Update; use Sylius\Resource\Metadata\Operations; use App\Entity\Speaker; return new ResourceMetadata() ->withClass(Speaker::class) ->withOperations(new Operations([ new Create(), new Update(), ])) ; ```
| Q | A | --------------- | ----- | Bug fix? | no | New feature? | no | BC breaks? | no | Deprecations? | no | Related tickets | | License | MIT
…rom ^4.2.1 || ^5.1 to ^6.1.0 --- updated-dependencies: - dependency-name: matthiasnoback/symfony-dependency-injection-test dependency-version: 6.1.0 dependency-type: direct:development ... Signed-off-by: dependabot[bot] <[email protected]>
| Q | A | --------------- | ----- | Bug fix? | no/yes | New feature? | no/yes | BC breaks? | no/yes | Deprecations? | no/yes <!-- don't forget to update the UPGRADE-*.md file --> | Related tickets | | License | MIT ### Documentation We'll need to add that new variable on this documentation https://stack.sylius.com/resource/index/resource_factories#pass-arguments-to-your-method ### **Example of usage** Creating a taxon from a parent The path is `/admin/taxons/new/{id}` So the `id` comes from the request attributes. **Previous implementation** ```yaml sylius_admin_taxon_create_for_parent: # ... defaults: _controller: sylius.controller.taxon::createAction _sylius: # ... factory: method: createForParent arguments: ['expr:notFoundOnNull(service("sylius.repository.taxon").find($id))'] ``` `service` is a function which can retrieve "public" services. The public services are not the recommanded way. Due to performance issues, the services are private by default. `$id` is parsed with the Sylius\Bundle\ResourceBundle\Provider\RequestParameterProvider which is kind of magic, and the syntax with a "$" is a little bit strange. **New implementation** ```php use Sylius\Bundle\AdminBundle\Form\Type\TaxonType; use Sylius\Resource\Metadata\BulkDelete; use Sylius\Resource\Metadata\Create; use Sylius\Resource\Metadata\Delete; use Sylius\Resource\Metadata\Operations; use Sylius\Resource\Metadata\ResourceMetadata; use Sylius\Resource\Metadata\Update; return (new ResourceMetadata()) // ... ->withOperations(new Operations([ // ... new Create( // ... factoryMethod: 'createForParent', factoryArguments: ['parent' => "throw_not_found_on_null(sylius_repositories.get('sylius.repository.taxon').find(request.attributes.get('id')))"], ), ])) ; ``` With that new implementation for the new routing system, it used sth that is more natural in Symfony applications and with current Expression language usages that we can see on serveral Symfony packages. You can use the `request` object directly with all its methods. You can use all the Sylius repositories to find an object and passed it as an argument to your factory method. In addition, I think it could be cool to add sth more generic for Symfony applications with Doctrine. Accessing entity_manager directly is not safe, cause there are methods to flush... or things that we do not want to have access here. Maybe a `doctrine_repository(Dummy:class)` function, could be perfect for this.
| Q | A | --------------- | ----- | Bug fix? | yes (but it can happen only on 1.14 branch) | New feature? | no | BC breaks? | no | Deprecations? | no | Related tickets | | License | MIT When we define a resource via an external php file, we registers the resource as a Sylius one. But there will be no form by default which should be ok. With autoregistration, or with a Resource from Sylius resource bundle config, the form type will be the `DefaultResourceType`. I think that'd be cool to remove that default form type and use it only if you need to (for RAD purpose).
| Q | A
| --------------- | -----
| Bug fix? | no
| New feature? | yes
| BC breaks? | no
| Deprecations? | no
| Related tickets |
| License | MIT
```php
<?php
use Sylius\Resource\Metadata\Create;
use Sylius\Resource\Metadata\Operations;
use Sylius\Resource\Metadata\ResourceMetadata;
return (new ResourceMetadata())
// ...
->withOperations(operations: new Operations(operations: [
new Create(
// ...
factoryMethod: 'createForProduct',
factoryArguments: [
"throw_not_found_on_null(sylius_repositories.get('sylius.repository.product').find(request.attributes.get('productId'))), 'Product not found."
],
),
]))
;
```
…rom ^4.2.1 || ^5.1 to ^6.1.0 (#1027) Dependabot will resolve any conflicts with this PR as long as you don't alter it yourself. You can also trigger a rebase manually by commenting `@dependabot rebase`. [//]: # (dependabot-automerge-start) [//]: # (dependabot-automerge-end) --- <details> <summary>Dependabot commands and options</summary> <br /> You can trigger Dependabot actions by commenting on this PR: - `@dependabot rebase` will rebase this PR - `@dependabot recreate` will recreate this PR, overwriting any edits that have been made to it - `@dependabot merge` will merge this PR after your CI passes on it - `@dependabot squash and merge` will squash and merge this PR after your CI passes on it - `@dependabot cancel merge` will cancel a previously requested merge and block automerging - `@dependabot reopen` will reopen this PR if it is closed - `@dependabot close` will close this PR and stop Dependabot recreating it. You can achieve the same result by closing it manually - `@dependabot show <dependency name> ignore conditions` will show all of the ignore conditions of the specified dependency - `@dependabot ignore this major version` will close this PR and stop Dependabot creating any more for this major version (unless you reopen the PR or upgrade to it yourself) - `@dependabot ignore this minor version` will close this PR and stop Dependabot creating any more for this minor version (unless you reopen the PR or upgrade to it yourself) - `@dependabot ignore this dependency` will close this PR and stop Dependabot creating any more for this dependency (unless you reopen the PR or upgrade to it yourself) </details>
This PR has been generated automatically. For more details see [upmerge_pr.yaml](/Sylius/SyliusResourceBundle/blob/1.13/.github/workflows/upmerge_pr.yaml). **Remember!** The upmerge should always be merged with using `Merge pull request` button. In case of conflicts, please resolve them manually with usign the following commands: ``` git fetch upstream gh pr checkout <this-pr-number> git merge upstream/1.14 -m "Resolve conflicts between 1.13 and 1.14" ``` If you use other name for the upstream remote, please replace `upstream` with the name of your remote pointing to the `Sylius/SyliusResourceBundle` repository. Once the conflicts are resolved, please run `git merge --continue` and push the changes to this PR.
| Q | A | --------------- | ----- | Bug fix? | no | New feature? | no | BC breaks? | no | Deprecations? | no | Related tickets | no | License | MIT
Co-authored-by: Dmitri Perunov <[email protected]>
| Q | A
| --------------- | -----
| Bug fix? | no
| New feature? | yes
| BC breaks? | no
| Deprecations? | no
| Related tickets |
| License | MIT
The feature we need with the new routing system
**Previous implementation:**
```yaml
sylius_admin_catalog_promotion_product_variant_index:
path: /catalog-promotions/{id}/variants
defaults:
_sylius:
vars:
catalogPromotion: expr:service('sylius.repository.catalog_promotion').find($id)
```
**New implementation:**
```php
new Index(vars: [
'catalogPromotion' => "@=sylius_repositories.get('sylius.repository.catalog_promotion').find(request.attributes.get('id')",
]);
```
**Note**
`@=` is the synthax for expression that is used on Symfony dependency
injection and Twig Hooks
Example on Sylius E-commerce admin panel
The product is used on the breadcrumb.
This PR has been generated automatically. For more details see [upmerge_pr.yaml](/Sylius/SyliusResourceBundle/blob/1.13/.github/workflows/upmerge_pr.yaml). **Remember!** The upmerge should always be merged with using `Merge pull request` button. In case of conflicts, please resolve them manually with usign the following commands: ``` git fetch upstream gh pr checkout <this-pr-number> git merge upstream/1.14 -m "Resolve conflicts between 1.13 and 1.14" ``` If you use other name for the upstream remote, please replace `upstream` with the name of your remote pointing to the `Sylius/SyliusResourceBundle` repository. Once the conflicts are resolved, please run `git merge --continue` and push the changes to this PR.
| Q | A | --------------- | ----- | Bug fix? | no | New feature? | yes | BC breaks? | no | Deprecations? | no | Related tickets | | License | MIT On Sylius, it uses grid filters on redirectToIndex action. https://github.com/Sylius/Sylius/blob/fb47637cef02d7573553b84315e1899f77221699/src/Sylius/Bundle/AdminBundle/Controller/RedirectHandler.php#L35 The new routing system has a new RedirectHandler service, so we need to implement this feature again. Test when redirecting the create product to update operation. 
| Q | A
| --------------- | -----
| Bug fix? | no
| New feature? | yes
| BC breaks? | no
| Deprecations? | no
| Related tickets |
| License | MIT
To create a bc-layer for the legacy routes in Sylius E-commerce project,
I need to add some route conditions to existing routes.
On simple route, I'm able to create this condition:
```yaml
sylius_admin_product_create_simple:
path: /products/new/simple
methods: [GET, POST]
condition: "context.isSyliusRoutingBcLayerEnabled() || context.isSyliusRoutingBcLayerEnabled('admin_product')"
# ...
```
But I also need to add this condition on multiple routing definition
like this following one and that's the reason of this PR.
```yaml
sylius_admin_product:
resource: |
alias: sylius.product
condition: "context.isSyliusRoutingBcLayerEnabled() || context.isSyliusRoutingBcLayerEnabled('admin_product')"
# ...
type: sylius.resource
```
To bring more context, here is the RequestContext we will implement on
Sylius core to create the bc-layer
```php
namespace Sylius\Bundle\CoreBundle\Routing;
use Symfony\Component\Routing\RequestContext as BaseRequestContext;
final class RequestContext extends BaseRequestContext
{
public function __construct(
BaseRequestContext $decorated,
private array $bcLayerConfig,
) {
// ...
}
public function isSyliusRoutingBcLayerEnabled(?string $key = null): bool
{
if (null === $key) {
return $this->bcLayerConfig['enabled'] ?? false;
}
return $this->bcLayerConfig['routes'][$key]['enabled'] ?? false;
}
}
```
| Q | A | --------------- | ----- | Bug fix? | no | New feature? | yes | BC breaks? | no | Deprecations? | no | Related tickets | | License | MIT The system works for the bc-layer usage. We just need to have two route names ```php new Index(routeName: '_sylius_admin_product_index', grid: 'sylius_admin_product'), ``` **bc-layer disabled**  **bc-layer enabled** 
| Q | A
| --------------- | -----
| Bug fix? | no
| New feature? | yes
| BC breaks? | no
| Deprecations? | no
| Related tickets |
| License | MIT
To allow both new and old routing systems working together, we defines
new route conditions.
**on legacy routes**
```
sylius_admin_product:
resource: |
# ...
condition: "context.isSyliusRoutingBcLayerEnabled('admin_product')"
type: sylius.resource
```
**on new routes**
```php
return (new ResourceMetadata())
>withClass('%sylius.model.product.class%')
// ...
->withRouteCondition("not context.isSyliusRoutingBcLayerEnabled('admin_product')")
;
```
But to make that works, we need to have two different route names.
Otherwise, there is only one Symfony route with the same condition.
Here is an example of what we need:
- legacy one: `sylius_admin_product_index` (we keep the orginal name)
- new one: `_sylius_admin_product_index` (we prefix the name).
**bc-layer disabled**
**bc-layer enabled**
But we need the **both paths match exactly** (and http methods too).
That's the reason of that current PR.
**Bc-layer enabled**
<img width="840" height="129" alt="image"
src="https://github.com/user-attachments/assets/0cba7f37-8a81-4a25-becf-b4f653e0a090"
/>
**Bc-layer disabled**
<img width="862" height="133" alt="image"
src="https://github.com/user-attachments/assets/84639e9d-8d66-4d66-ad12-5e127e5f3aae"
/>
That seems to work on Sylius E-commerce. The two errors I have are
solved in another PR.
<img width="1510" height="1206" alt="image"
src="https://github.com/user-attachments/assets/6b066851-2bcf-46e3-8a9f-fe63b1305064"
/>
| Q | A | --------------- | ----- | Bug fix? | yes | New feature? | yes | BC breaks? | no | Deprecations? | no | Related tickets | | License | MIT This will handle this part of the resource controller: https://github.com/Sylius/SyliusResourceBundle/blob/1.14/src/Bundle/Controller/ResourceController.php#L356 It will fix that error on Sylius E-commerce:  To allow throwing exceptions on writing process, we should move the flash processing directly on the write processor. It's easier to catch error and add the right flash message. Then writing failure will still call the respond processing. **After** 
| Q | A
| --------------- | -----
| Bug fix? | no
| New feature? | yes
| BC breaks? | no
| Deprecations? | no
| Related tickets |
| License | MIT
I'd like to be able to configure those redirect arguments:
```php
new Update(
methods: ['PATCH', 'POST'],
routeName: '_sylius_admin_promotion_archive',
shortName: 'archive',
formType: ArchivableType::class,
notificationMessage: 'sylius.resource.update',
redirectToRoute: 'sylius_admin_promotion_index',
redirectArguments: [
'criteria' => [
'archival' => 0,
],
],
)
```
for that Sylius feature:
```gherkin
@api @ui
Scenario: Restoring an archival promotion
Given the promotion "Christmas sale" is archived
When I browse promotions
And I filter archival promotions
And I restore the "Christmas sale" promotion
Then I should be viewing non archival promotions
And I should see 2 promotions on the list
```
…nse (#1057) | Q | A | --------------- | ----- | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Related tickets | | License | MIT When data returned by the write processor is already a response, we does not have to dispatch a post write event. <img width="914" height="267" alt="image" src="https://github.com/user-attachments/assets/5a949988-3571-498d-b8cd-4a8bf0361124" />
The class scanner used a simple `/class\s+(\w+)/` regex on raw file contents. When the word "class" appeared in comments or strings (e.g. "grid class you have generated"), the regex captured "you" as a class name, leading to errors such as: "App\Entity\you" This fixes the issue encountered when following the Sylius Stack documentation (Basic Operations cookbook): https://stack.sylius.com/cookbook/admin_panel/basic_operations
|
@dario-dib thx for your PR, could you rebase it to 1.13 branch please? |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The class scanner used a simple
/class\s+(\w+)/regex on raw file contents. When the word "class" appeared in comments or strings (e.g. "grid class you have generated"), the regex captured "you" as a class name, leading to errors such as: "App\Entity\you"This fixes the issue encountered when following the Sylius Stack documentation (Basic Operations cookbook): https://stack.sylius.com/cookbook/admin_panel/basic_operations