Skip to content
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

[Debug] Fix false-positive "MicroKernelTrait::loadRoutes()" method is considered internal" #28553

Merged
merged 1 commit into from Sep 22, 2018

Conversation

Projects
None yet
3 participants
@nicolas-grekas
Copy link
Member

commented Sep 22, 2018

Q A
Branch? 3.4
Bug fix? yes (fixing and unreleased issue)
New feature? no
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets #28549
License MIT
Doc PR -

Introduced in #28411
Just a failing test for now.

@GuilhemN

This comment has been minimized.

Copy link
Contributor

commented Sep 22, 2018

I've done the fix in master...GuilhemN:debug-int, I did not realize it was there since 3.4...
BTW one of test cases should have detected this issue... If it wasn't broken itself 😅

@nicolas-grekas nicolas-grekas force-pushed the nicolas-grekas:debug-int branch 2 times, most recently from 88d6c25 to 4164755 Sep 22, 2018

@nicolas-grekas nicolas-grekas force-pushed the nicolas-grekas:debug-int branch from 4164755 to 46c4f71 Sep 22, 2018

@nicolas-grekas

This comment has been minimized.

Copy link
Member Author

commented Sep 22, 2018

'@GuilhemN thanks a lot for having a look! Actually I don't agree with your patch, the test was fine and it's exactly why it was added in #28411. The issue is nastier: we don't deal with traits correctly. This very check you moved up shouldn't exist at all - and we should never inspect method annotations on traits.
This PR is now ready with the correct fix, see https://github.com/symfony/symfony/pull/28553/files?w=1

@GuilhemN

This comment has been minimized.

Copy link
Contributor

commented Sep 22, 2018

Oops sorry, I thought I was the one who introduced it and by mistake 😶 I understand now why you've done this change, I will review your last commit asap :)

@nicolas-grekas nicolas-grekas merged commit 46c4f71 into symfony:3.4 Sep 22, 2018

2 of 3 checks passed

continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
fabbot.io Your code looks good.
Details

nicolas-grekas added a commit that referenced this pull request Sep 22, 2018

bug #28553 [Debug] Fix false-positive "MicroKernelTrait::loadRoutes()…
…" method is considered internal" (nicolas-grekas)

This PR was merged into the 3.4 branch.

Discussion
----------

[Debug] Fix false-positive "MicroKernelTrait::loadRoutes()" method is considered internal"

| Q             | A
| ------------- | ---
| Branch?       | 3.4
| Bug fix?      | yes (fixing and unreleased issue)
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | #28549
| License       | MIT
| Doc PR        | -

Introduced in #28411
Just a failing test for now.

Commits
-------

46c4f71 [Debug] Fix false-positive "MicroKernelTrait::loadRoutes()" method is considered internal"

@nicolas-grekas nicolas-grekas deleted the nicolas-grekas:debug-int branch Sep 22, 2018

@GuilhemN
Copy link
Contributor

left a comment

Reviewed and looks all good to me 👍

@nicolas-grekas

This comment has been minimized.

Copy link
Member Author

commented Sep 22, 2018

@GuilhemN thanks for the review. Now merged up to master and green :)

This was referenced Sep 30, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.