Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

[DoctrineBridge] Don't use object IDs in DoctrineChoiceLoader when passing a value closure#18924

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Merged
fabpot merged 1 commit intosymfony:2.7fromwebmozart:fix-doctrine-choice-loader
Jun 22, 2016

Conversation

@webmozart
Copy link
Contributor

QA
Branch?2.7
Bug fix?yes
New feature?no
BC breaks?no
Deprecations?no
Tests pass?yes
Fixed tickets-
LicenseMIT
Doc PR-

This PR is porting an optimization done forloadChoicesForValues() in64c80a6 toloadValuesForChoices().

@javiereguiluz
Copy link
Member

javiereguiluz commentedMay 31, 2016•
edited
Loading

We recently introduced aMaintenance guide for contributions. It's so new that we're still adapting to it, so I'd like to ask something:

  • Does this contribution fit in the allowed"Performance improvement" category?
  • If yes, should we enforce this?"[...] any such patches must come with numbers that show a significant improvement on real-world code [...]"

To me in this case it would be overkill ... but then perhaps we need to tweak the Maintenance guide.

@webmozartwebmozartforce-pushed thefix-doctrine-choice-loader branch fromedddb1e to6c9dcc0CompareJune 1, 2016 10:34
@webmozart
Copy link
ContributorAuthor

@javiereguiluz This fix is not a performance improvement, but a bug fix where the performance improvement was applied where it shouldn't be (i.e. whenchoice_value is set). I added a test for DoctrineChoiceLoader that tests the bug fix.

@webmozartwebmozartforce-pushed thefix-doctrine-choice-loader branch from6c9dcc0 toeefafc5CompareJune 1, 2016 10:36
@javiereguiluz
Copy link
Member

javiereguiluz commentedJun 1, 2016•
edited
Loading

@webmozart thanks for the info. Everything is fine then!

@xabbuh
Copy link
Member

Looks like not all tests pass with this change.

Status: Needs work

$this->idReader
);

$value = [$this->idReader, 'getIdValue'];
Copy link
Member

Choose a reason for hiding this comment

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

should bearray(...) for PHP 5.3 compat.

@fabpot
Copy link
Member

@webmozart Can you have a look at the failing tests?

@webmozartwebmozartforce-pushed thefix-doctrine-choice-loader branch frome5ed2b0 tof6e5298CompareJune 22, 2016 09:11
@webmozart
Copy link
ContributorAuthor

Fixed

@fabpot
Copy link
Member

Thank you@webmozart.

@fabpotfabpot merged commitf6e5298 intosymfony:2.7Jun 22, 2016
fabpot added a commit that referenced this pull requestJun 22, 2016
…der when passing a value closure (webmozart)This PR was merged into the 2.7 branch.Discussion----------[DoctrineBridge] Don't use object IDs in DoctrineChoiceLoader when passing a value closure| Q             | A| ------------- | ---| Branch?       | 2.7| Bug fix?      | yes| New feature?  | no| BC breaks?    | no| Deprecations? | no| Tests pass?   | yes| Fixed tickets | -| License       | MIT| Doc PR        | -This PR is porting an optimization done for `loadChoicesForValues()` in64c80a6 to `loadValuesForChoices()`.Commits-------f6e5298 [DoctrineBridge] Don't use object IDs in DoctrineChoiceLoader when passing a value closure
This was referencedJun 30, 2016
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@webmozart@javiereguiluz@xabbuh@fabpot@carsonbot
[8]ページ先頭
©2009-2025 Movatter.jp