8000 [DoctrineBridge] Don't use object IDs in DoctrineChoiceLoader when passing a value closure by webmozart · Pull Request #18924 · symfony/symfony · GitHub
[go: up one dir, main page]

Skip to content

[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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Jun 22, 2016

Conversation

webmozart
Copy link
Contributor
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() in 64c80a6 to loadValuesForChoices().

@javiereguiluz
Copy link
Member
javiereguiluz commented May 31, 2016
Loading

We recently introduced a Maintenance 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.

@webmozart webmozart force-pushed the fix-doctrine-choice-loader branch from edddb1e to 6c9dcc0 Compare June 1, 2016 10:34
@webmozart
Copy link
Contributor Author

@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. when choice_value is set). I added a test for DoctrineChoiceLoader that tests the bug fix.

@webmozart webmozart force-pushed the fix-doctrine-choice-loader branch from 6c9dcc0 to eefafc5 Compare June 1, 2016 10:36
@javiereguiluz
Copy link
Member
javiereguiluz commented Jun 1, 2016

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

@xabbuh
Copy link
Member
xabbuh commented Jun 5, 2016

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 be array(...) for PHP 5.3 compat.

@fabpot
Copy link
Member
fabpot commented Jun 22, 2016

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

@webmozart webmozart force-pushed the fix-doctrine-choice-loader branch from e5ed2b0 to f6e5298 Compare June 22, 2016 09:11
@webmozart
Copy link
Contributor Author

Fixed

@fabpot
Copy link
Member
fabpot commented Jun 22, 2016

Thank you @webmozart.

@fabpot fabpot merged commit f6e5298 into symfony:2.7 Jun 22, 2016
fabpot added a commit that referenced this pull request Jun 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()` in 64c80a6 to `loadValuesForChoices()`.

Commits
-------

f6e5298 [DoctrineBridge] Don't use object IDs in DoctrineChoiceLoader when passing a value closure
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.

5 participants
0