[3.3] Glob loader breaks loading inline YAML resources #22938
Comments
|
@alcaeus it's a BC break in 3.3 for sure, but one we want to do IMHO. Still, we can enhance the glob-detection logic to make it more specific. Detecting newlines as you suggested. |
|
You'd have to ask one of the Sylius core devs that, maybe @pamil or @lchrusciel can answer that. Don't tell me you could actually write this like so: |
|
@nicolas-grekas Currently, |
|
@xabbuh the type hint on LoaderInterface and LoaderResolverInterface says "mixed", so should be good, isn't it? |
|
Oh indeed, the loaders should still work fine. However, you will run into issues when defining routes with XML as the |
|
See #22940. |
|
That was quick, thanks! |
|
@alcaeus @nicolas-grekas it was a hacky solution to allow defining nested yaml instead of string only. Otherwise, something has been broken. @pjedrzejewski has implemented this feature, so he will have the best knowledge about it. |
…as-grekas) This PR was merged into the 4.0-dev branch. Discussion ---------- [Config] Fallback to regular import when glob fails | Q | A | ------------- | --- | Branch? | 3.3 | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | #22938 | License | MIT | Doc PR | - Fixes a BC break as reported in linked issue. Commits ------- 29802a2 [Config] Fallback to regular import when glob fails
…as-grekas) This PR was submitted for the master branch but it was merged into the 3.3 branch instead (closes #22940). Discussion ---------- [Config] Fallback to regular import when glob fails | Q | A | ------------- | --- | Branch? | 3.3 | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | #22938 | License | MIT | Doc PR | - Fixes a BC break as reported in linked issue. Commits ------- 7b4066e [Config] Fallback to regular import when glob fails
|
Still can't use with something like this: sylius_admin_user:
resource: |
alias: sylius.admin_user
section: admin
path: users
templates: AppBundle:Resouce/Crud
type: sylius.resourceIt's work when remove
Other example: # Not work
sylius_admin_api_carts:
resource: |
alias: sylius.order
path: '/'
only: [index]
grid: sylius_admin_api_cart
section: admin_api
serialization_version: $version
type: sylius.resource_api
# Work
sylius_admin_api_carts:
resource: |
alias: sylius.order
#path: '/'
only: [index]
grid: sylius_admin_api_cart
section: admin_api
serialization_version: $version
type: sylius.resource_apiConclusion We can't use PS. Got error when |
|
@liverbool Please open a new issue if you think you are running in a bug here with some information on how to reproduce your issue. |
…elKaefer) This PR was merged into the 4.4 branch. Discussion ---------- [Config] Stop treating multiline resources as globs | Q | A | ------------- | --- | Branch? | 4.4 | Bug fix? | yes | New feature? | no | Deprecations? | no | Tickets | Fix #23412 | License | MIT Would fix the linked issue. In #22938 it was suggested to enhance the glob-detection logic by detecting newlines. Cons: - it only solves an edge case - it is not possible to use a multiline glob (like `bar\nbaz*.txt`) as a resource anymore - maybe in another edge case this is needed Commits ------- 1e3baad 23412 Stop treating multiline resources as globs
…elKaefer) This PR was merged into the 4.4 branch. Discussion ---------- [Config] Stop treating multiline resources as globs | Q | A | ------------- | --- | Branch? | 4.4 | Bug fix? | yes | New feature? | no | Deprecations? | no | Tickets | Fix #23412 | License | MIT Would fix the linked issue. In symfony/symfony#22938 it was suggested to enhance the glob-detection logic by detecting newlines. Cons: - it only solves an edge case - it is not possible to use a multiline glob (like `bar\nbaz*.txt`) as a resource anymore - maybe in another edge case this is needed Commits ------- 1e3baad386 23412 Stop treating multiline resources as globs
First of all, I'm not sure if this is issue or if this "just shouldn't be done this way".
Upgrading to Symfony 3.3.0-RC1 in Sylius causes an error as indicated in Sylius/Sylius#7896. After additional investigation I figured this is caused due to Sylius using a custom loader to load config files with inline YAML as a
resource(example shortened):The custom loader generates routes based on the configuration given in the
resourcekey, so it's a nice way to avoid writing lots of boilerplate code for the same thing over and over. Using theresourcekey also reduces the amount of files needed: without this, there would have to be one routing file and another file containing the actual specification (now contained inresource).With this comes the issue: the introduction of glob loaders in #21635 broke this completely, while #22621 partially fixed it: it no longer breaks for config files that don't contain any of the characters triggering the glob loader (removing the
exceptdirective in the config above fixes the error for this file).Would you consider this a valid use-case of the
resourcekey in a config file? If so, should the check for the glob pattern be hardened? In this case, adding a check against newlines would help, but who knows what else is out there. Another option would be to fall back to the default behavior if the glob loader didn't load anything, but I'm also not sure what side-effects this might have. Last but not least, this could also be fixed on the Sylius side; I just wanted to check here before doing that.The text was updated successfully, but these errors were encountered: