Skip to content

[Routing][Config] Allow patterns of resources to be excluded from config loading #31587

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged

Conversation

tristanbes
Copy link
Contributor

@tristanbes tristanbes commented May 22, 2019

Q A
Branch? master
Bug fix? no
New feature? yes
BC breaks? no
Deprecations? yes
Tests pass? yes
Fixed tickets #31516
License MIT
Doc PR not yet

The PR will fix the following RFC: #31516

Like resource loading for services, this PR offers a way to exclude patterns of resources like:

// config/routes/annotations.yaml
controllers:
    resource: ../../src/Controller/*
    type: annotation
    exclude: '../src/Controller/{DebugEmailController}.php'

All the annotation routes inside Controller/ will be loaded in this example except all the one present inside the Controller/DebugEmailController.php

@vudaltsov
Copy link
Contributor

If you change FileLoader in the Config component, you should test the new behavior in the Config component in the first place.

@vudaltsov
Copy link
Contributor

Deprecations for 4.4 must be described in UPGRADE-4.4.md (create if it does not exist), BC breaks must be mentioned in UPGRADE-5.0.md. Duplicate this info in src/Symfony/Component/Config/CHANGELOG.md.

@tristanbes tristanbes marked this pull request as ready for review May 27, 2019 14:47
@tristanbes tristanbes force-pushed the feature/allow-exclude-controllers branch from fe50d64 to 3ffd4ee Compare May 27, 2019 15:19
@tristanbes tristanbes changed the title [routing] Exclude patterns of resources to be excluded from route loading [routing][config] Allow patterns of resources to be excluded from config loading May 27, 2019
@nicolas-grekas nicolas-grekas changed the base branch from master to 4.4 June 2, 2019 20:12
@tristanbes tristanbes force-pushed the feature/allow-exclude-controllers branch 2 times, most recently from 3ffd4ee to af14114 Compare June 6, 2019 16:33
Copy link
Member

@nicolas-grekas nicolas-grekas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what about RoutingConfigurator + XmlFileLoader?
also, do we want this in the DI component, where we also extend FileLoader?
rebase needed btw :)

@tristanbes tristanbes force-pushed the feature/allow-exclude-controllers branch from af14114 to c15a7fe Compare July 31, 2019 11:23
@tristanbes
Copy link
Contributor Author

@nicolas-grekas I've just added the modifications for the RoutingConfigurator are the modifications correct ?

Also, I didn't find any BC rules reguarding a final method inside a non final class, so i'm not sure the $exclude parameter should be retrieved using fun_get_arg.

@nicolas-grekas
Copy link
Member

Yes, you can change a final method, no need for the func_get_arg trick

@nicolas-grekas
Copy link
Member

we can bc break a final function

No, we can't. This is not a BC break, that's why this change is OK :)

@tristanbes tristanbes force-pushed the feature/allow-exclude-controllers branch from ce09b71 to 96579e8 Compare July 31, 2019 12:25
@nicolas-grekas nicolas-grekas changed the title [routing][config] Allow patterns of resources to be excluded from config loading [Routing][Config] Allow patterns of resources to be excluded from config loading Sep 27, 2019
@nicolas-grekas nicolas-grekas force-pushed the feature/allow-exclude-controllers branch from 96579e8 to c21f67b Compare September 27, 2019 18:53
Copy link
Member

@nicolas-grekas nicolas-grekas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I took over the PR and finished the implementation. fabbot failure is a false-positive

@tristanbes
Copy link
Contributor Author

@nicolas-grekas Great, thank you; I just learnt that we're 1 week away from feature freeze ;)

@tristanbes
Copy link
Contributor Author

is there anything missing to get this PR merged ?

@tristanbes tristanbes force-pushed the feature/allow-exclude-controllers branch from c21f67b to 332ff88 Compare October 24, 2019 11:56
@tristanbes
Copy link
Contributor Author

I solved the conflicts on the PR. @fabpot is this PR good to merge ?

@nicolas-grekas nicolas-grekas modified the milestones: next, 4.4 Oct 27, 2019
@nicolas-grekas
Copy link
Member

Thank you @tristanbes.

nicolas-grekas added a commit that referenced this pull request Nov 4, 2019
…cluded from config loading (tristanbes)

This PR was merged into the 4.4 branch.

Discussion
----------

[Routing][Config] Allow patterns of resources to be excluded from config loading

| Q             | A
| ------------- | ---
| Branch?       | master
| Bug fix?      | no
| New feature?  | yes
| BC breaks?    | no
| Deprecations? | yes
| Tests pass?   | yes
| Fixed tickets | #31516
| License       | MIT
| Doc PR        | not yet

The PR will fix the following RFC: #31516

Like resource loading for services, this PR offers a way to exclude patterns of resources like:

```yml
// config/routes/annotations.yaml
controllers:
    resource: ../../src/Controller/*
    type: annotation
    exclude: '../src/Controller/{DebugEmailController}.php'
```

All the annotation routes inside `Controller/` will be loaded in this example except all the one present inside the `Controller/DebugEmailController.php`

Commits
-------

332ff88 [Routing][Config] Allow patterns of resources to be excluded from config loading
@nicolas-grekas nicolas-grekas merged commit 332ff88 into symfony:4.4 Nov 4, 2019
@tristanbes tristanbes deleted the feature/allow-exclude-controllers branch November 4, 2019 12:36
This was referenced Nov 12, 2019
if (\is_string($resource) && \strlen($resource) !== $i = strcspn($resource, '*?{[')) {
$excluded = [];
Copy link
Contributor

@gharlan gharlan Feb 11, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it expected, that exclude is only handled when the resource contains glob patterns (*?{[)?
exclude does not have any effect when using a directory as resource, like in default recipe:
https://github.com/symfony/recipes/blob/b364e60751fba4fc5da103304c1f61259d370e10/doctrine/annotations/1.0/config/routes/annotations.yaml#L2

Copy link

@Growiel Growiel Jun 10, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I stumbled upon the same issue and it cost me quite a lot of time to figure it out !

I do not believe this is working as intended, as it's different from what the documentation says.

To get my exclude to work, I had to change my resource too (add *):

controllers:
    resource: '../../src/Controller'
    exclude: '../../src/Controller/Backoffice/{HomepageController}.php'

does NOT work (HomepageController is not excluded)..

controllers:
    resource: '../../src/Controller/*'
    exclude: '../../src/Controller/Backoffice/{HomepageController}.php'

does work, the HomepageController is correctly excluded.

Without the *, the condition is not satisfied and the exclude is ignored totally, which makes no sense.

Should we open a new issue for this, @tristanbes ?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thx @gharlan and @Growiel I stumbled upon this strange behaviour today and thanks to your hints I did not have to spend too much time debugging this :) Did you open an issue for this though?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hello, I guess you should open an issue if you think it's a bug or if this case should be handled

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is indeed a bug. Do you need a new issue or handle it here ?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please create a new issue. Comments on already merged pull requests are likely to get lost.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants