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.

Sign up for GitHub

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

Jump to bottom

fix api-platform/#360 QueryChecker with class join #1301

Merged
merged 1 commit into from
Sep 7, 2017
Merged

fix api-platform/#360 QueryChecker with class join #1301

merged 1 commit into from
Sep 7, 2017

Conversation

soyuka
Copy link
Member

@soyuka soyuka commented Aug 2, 2017

Q A
Bug fix? yes
New feature? no
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets api-platform/api-platform#360
License MIT
Doc PR na

@soyuka soyuka requested a review from Simperfit August 2, 2017 12:57
soyuka
soyuka commented Aug 2, 2017
View reviewed changes
src/Bridge/Doctrine/Orm/Util/QueryChecker.php Outdated

$parentMetadata = QueryJoinParser::getClassMetadataFromJoinAlias($parentAlias, $queryBuilder, $managerRegistry);
if (class_exists($relationship)) {
Copy link
Member Author

Choose a reason for hiding this comment

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

Not sure this is the best thing to do though

soyuka
soyuka commented Aug 2, 2017
View reviewed changes
src/Bridge/Doctrine/Orm/Util/QueryChecker.php Outdated
list($parentAlias, $association) = explode('.', $relationship);

$parentMetadata = QueryJoinParser::getClassMetadataFromJoinAlias($parentAlias, $queryBuilder, $managerRegistry);
}
Copy link
Member Author

Choose a reason for hiding this comment

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

need to reverse condition, will be cleaner

@soyuka soyuka force-pushed the fix/api-platform/360 branch 3 times, most recently from 4d68d98 to b40c191 Compare August 4, 2017 14:07
meyerbaptiste
meyerbaptiste reviewed Aug 7, 2017
View reviewed changes
src/Bridge/Doctrine/Orm/Util/QueryChecker.php Outdated

$parentMetadata = QueryJoinParser::getClassMetadataFromJoinAlias($parentAlias, $queryBuilder, $managerRegistry);
if (!class_exists($relationship)) {
Copy link
Member

Choose a reason for hiding this comment

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

IMO a regex is better than the class_exists method which use the autoloading:

if (!preg_match('/^[a-zA-Z_\x7f-\xff][a-zA-Z0-9_\x7f-\xff]*+(?:\\\\[a-zA-Z_\x7f-\xff][a-zA-Z0-9_\x7f-\xff]*+)*+$/', $relationship)) {

Copy link
Member Author

Choose a reason for hiding this comment

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

Hmm yes, or we can do:

  • a try/catch thing
  • test if relation is in the association mapping
  • strpos for a dot in the realtionship

Copy link
Member

Choose a reason for hiding this comment

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

strpos for a dot in the realtionship

Sounds good and better for performance since dots are not allowed in FQCN!

Copy link
Member Author

Choose a reason for hiding this comment

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

hmm not sure strpos is sufficient because getJoinRelationship returns the association alone:

        $join = new Join('INNER_JOIN', 'relatedDummy', 'a_1', null, 'a_1.name = r.name');
        $this->assertEquals('relatedDummy', QueryJoinParser::getJoinRelationship($join));

@soyuka soyuka force-pushed the fix/api-platform/360 branch from b40c191 to 210ffc6 Compare August 7, 2017 11:36
meyerbaptiste
meyerbaptiste approved these changes Aug 7, 2017
View reviewed changes
Copy link
Member

@meyerbaptiste meyerbaptiste left a comment

Choose a reason for hiding this comment

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

Just add a test for "dotted" joins!

@soyuka
Copy link
Member Author

soyuka commented Aug 7, 2017

getClassMetadataFromJoinAlias also needs a fix and relies on strpos($relationship, '.') without testing it for false...

@soyuka soyuka force-pushed the fix/api-platform/360 branch from 210ffc6 to b0b0089 Compare August 7, 2017 14:55
@soyuka
Copy link
Member Author

soyuka commented Aug 7, 2017

I've reviewed the code, there are a lot of assumptions in the QueryChecker/QueryJoinParser but I think it'll be fine. AFAIR we can't have an alias without . in a join relation (or it's a class). Based on this the updated code should be fine.

@teohhanhui
Copy link
Contributor

there are a lot of assumptions in the QueryChecker/QueryJoinParser

Guilty as charged. 😆

@soyuka soyuka mentioned this pull request Aug 11, 2017
Closed
dunglas
dunglas requested changes Aug 18, 2017
View reviewed changes
src/Bridge/Doctrine/Orm/Util/QueryChecker.php Outdated
if (empty($orderByAliases)) {
return false;
}

if (!empty($orderByAliases)) {
Copy link
Member

Choose a reason for hiding this comment

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

This test is useless now isn't it?

src/Bridge/Doctrine/Orm/Util/QueryChecker.php Outdated
@@ -135,20 +135,44 @@ public static function hasOrderByOnToManyJoin(QueryBuilder $queryBuilder, Manage
}
}

if (empty($orderByAliases)) {
Copy link
Member

Choose a reason for hiding this comment

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

if (!$orderByAliases)?

src/Bridge/Doctrine/Orm/Util/QueryChecker.php Outdated
->getManagerForClass($rootEntity)
->getClassMetadata($rootEntity);

if (!($rootMetadata instanceof ClassMetadata)) {
Copy link
Member

Choose a reason for hiding this comment

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

if (!$rootMetadata instanceof ClassMetadata) (useless parenthesis).

@soyuka soyuka force-pushed the fix/api-platform/360 branch 2 times, most recently from e285cc6 to 79f3615 Compare August 28, 2017 07:40
@soyuka soyuka force-pushed the fix/api-platform/360 branch from 79f3615 to 9f9da0c Compare September 5, 2017 14:06
@dunglas dunglas merged commit 4cd7b02 into api-platform:2.0 Sep 7, 2017
@dunglas
Copy link
Member

dunglas commented Sep 7, 2017

Thanks @soyuka!

@soyuka soyuka deleted the fix/api-platform/360 branch January 29, 2018 09:38
hoangnd25 pushed a commit to hoangnd25/core that referenced this pull request Feb 23, 2018
@dunglas
Merge pull request api-platform#1301 from soyuka/fix/api-platform/360
1545aef 
fix api-platform/api-platform#360 QueryChecker with class join
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@meyerbaptiste meyerbaptiste meyerbaptiste approved these changes

@Simperfit Simperfit Simperfit approved these changes

@dunglas dunglas dunglas approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@soyuka @teohhanhui @dunglas @Simperfit @meyerbaptiste