-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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
#5934 - add fetch
option to AssociationOverride
in order to override fetch strategy for subclasses of entities
#5938
#5934 - add fetch
option to AssociationOverride
in order to override fetch strategy for subclasses of entities
#5938
Conversation
<generator strategy="AUTO" /> | ||
</id> | ||
|
||
<many-to-many target-entity="DDC5934Member" inversed-by="contract" fetch="LAZY" field="members" /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a typo here? Should this be contracts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
possibly. Some tests are still failing. Try to fix it somehow in the next days...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no, not a typo ;)
ok, fixed the tests now and squashed everything into a single commit @Ocramius anything to add here? |
Looks good to me! LGTM! |
Sad that this didn't make the release. Anything else to do for this one @Ocramius? |
Patch releases don't get new features, folks! On 11 Sep 2016 5:05 a.m., "Patrick McDougle" notifications@github.com Sad that this didn't make the release. Anything else to do for this one — Reply to this email directly, view it on GitHub |
http://semver.org/ - the release yesterday was a release which shouldn't contain anything which is a new feature (and this one is definitly a new feature) |
Can we get this in 3.0? |
@patrick-mcdougle we'll see, 2.6 hopefully :-) |
Any news on this? |
…ide fetch strategy for subclasses of entities
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've rebased it to sync with master
, made a tiny change (to use ::class
syntax) and it LGTM
ok, thank you @lcobucci :-)
…On Tue, May 30, 2017 at 6:29 PM, Luís Cobucci ***@***.***> wrote:
***@***.**** approved this pull request.
I've rebased it to sync with master and made a tiny change (to use ::class
syntax) and it LGTM
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#5938 (review)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AFvwBHfXWf57NDx3vdjcShelzcSXqfnVks5r_EP7gaJpZM4JN-7o>
.
|
@@ -621,6 +621,11 @@ public function loadMetadataForClass($className, ClassMetadata $metadata) | |||
$override['inversedBy'] = (string) $overrideElement->{'inversed-by'}['name']; | |||
} | |||
|
|||
// Check for `fetch` | |||
if (isset($overrideElement['fetch'])) { | |||
$override['fetch'] = constant('Doctrine\ORM\Mapping\ClassMetadata::FETCH_' . (string) $overrideElement['fetch']); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we use ::class
for the first bit of the string?
@@ -470,6 +470,11 @@ public function loadMetadataForClass($className, ClassMetadata $metadata) | |||
$override['inversedBy'] = $associationOverride->inversedBy; | |||
} | |||
|
|||
// Check for `fetch` | |||
if ($associationOverride->fetch) { | |||
$override['fetch'] = constant('Doctrine\ORM\Mapping\ClassMetadata::FETCH_' . $associationOverride->fetch); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we use ::class
for the first bit of the string?
@@ -622,6 +622,11 @@ public function loadMetadataForClass($className, ClassMetadata $metadata) | |||
$override['inversedBy'] = (string) $associationOverrideElement['inversedBy']; | |||
} | |||
|
|||
// Check for `fetch` | |||
if (isset($associationOverrideElement['fetch'])) { | |||
$override['fetch'] = constant('Doctrine\ORM\Mapping\ClassMetadata::FETCH_' . $associationOverrideElement['fetch']); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we use ::class
for the first bit of the string?
thanks for the review, hope I'll find some time tonight to address your comments :-)
… On 31. May 2017, at 11:39 AM, Marco Pivetta ***@***.***> wrote:
@Ocramius requested changes on this pull request.
In lib/Doctrine/ORM/Mapping/Driver/XmlDriver.php:
> @@ -621,6 +621,11 @@ public function loadMetadataForClass($className, ClassMetadata $metadata)
$override['inversedBy'] = (string) $overrideElement->{'inversed-by'}['name'];
}
+ // Check for `fetch`
+ if (isset($overrideElement['fetch'])) {
+ $override['fetch'] = constant('Doctrine\ORM\Mapping\ClassMetadata::FETCH_' . (string) $overrideElement['fetch']);
Can we use ::class for the first bit of the string?
In lib/Doctrine/ORM/Mapping/Driver/AnnotationDriver.php:
> @@ -470,6 +470,11 @@ public function loadMetadataForClass($className, ClassMetadata $metadata)
$override['inversedBy'] = $associationOverride->inversedBy;
}
+ // Check for `fetch`
+ if ($associationOverride->fetch) {
+ $override['fetch'] = constant('Doctrine\ORM\Mapping\ClassMetadata::FETCH_' . $associationOverride->fetch);
Can we use ::class for the first bit of the string?
In lib/Doctrine/ORM/Mapping/Driver/YamlDriver.php:
> @@ -622,6 +622,11 @@ public function loadMetadataForClass($className, ClassMetadata $metadata)
$override['inversedBy'] = (string) $associationOverrideElement['inversedBy'];
}
+ // Check for `fetch`
+ if (isset($associationOverrideElement['fetch'])) {
+ $override['fetch'] = constant('Doctrine\ORM\Mapping\ClassMetadata::FETCH_' . $associationOverrideElement['fetch']);
Can we use ::class for the first bit of the string?
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub, or mute the thread.
|
…N in config drivers as of @Ocramius's review
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 👍
@@ -472,7 +472,7 @@ public function loadMetadataForClass($className, ClassMetadata $metadata) | |||
|
|||
// Check for `fetch` | |||
if ($associationOverride->fetch) { | |||
$override['fetch'] = constant('Doctrine\ORM\Mapping\ClassMetadata::FETCH_' . $associationOverride->fetch); | |||
$override['fetch'] = constant(ClassMetadata::class . '::FETCH_' . $associationOverride->fetch); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This won't work since ClassMetadata
points to Doctrine\Common\Persistence\Mapping\ClassMetadata
and it doesn't have the constants.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ouch, I'm sorry^^
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ARGH, good catch :S
…get of ::class expression
@lcobucci as travis-ci seems to not run on this PR, I just executed the corresponding tests on my machine:
|
@Ma27 travis passed! 👍 |
adds the
fetch
option to theassociation-override
sequence.see #5934