Skip to content

Hotfix/filter eager loading fetch eager#1028

Merged
dunglas merged 2 commits into
api-platform:2.0from
soyuka:hotfix/filter-eager-loading-FETCH-EAGER
Apr 7, 2017
Merged

Hotfix/filter eager loading fetch eager#1028
dunglas merged 2 commits into
api-platform:2.0from
soyuka:hotfix/filter-eager-loading-FETCH-EAGER

Conversation

@soyuka

@soyuka soyuka commented Apr 3, 2017

Copy link
Copy Markdown
Member
Q A
Bug fix? yes
New feature? no
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets na
License MIT
Doc PR na

ping @teohhanhui

@soyuka
soyuka force-pushed the hotfix/filter-eager-loading-FETCH-EAGER branch 2 times, most recently from 25159c7 to 5a52bf6 Compare April 3, 2017 10:17
private function toDQLString(string $dql): string
{
return preg_replace('/\\r\\n|\\n/', '', str_replace(' ', '', $dql));
return preg_replace(['/\s+/', '/\(\s/', '/\s\)/'], [' ', '(', ')'], $dql);

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

This should be better \o/

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Actually this is unsafe... It might change whitespace within a SQL literal (string). Lol...

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Of course, as said before it makes tests more readable :).

@soyuka
soyuka force-pushed the hotfix/filter-eager-loading-FETCH-EAGER branch from 5a52bf6 to 84f5d7f Compare April 3, 2017 10:24
return is_bool($forceEager) ? $forceEager : $this->forceEager;
}

private function hasFetchEagerAssociation(EntityManager $em, ClassMetadataInfo $classMetadata, &$checked = [])

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Is it possible to extract this logic out for reuse between EagerLoadingExtension and FilterEagerLoadingExtension?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I don't want to to this to avoid a 2-pass recursive call in EagerLoadingExtension. If we want to extract this logic we need to add cache. I obviously agree that we should add a new class with the shouldEagerLoad logic.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Can't this be extracted in another PR?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Yeah it can and should, +1 to do this in another PR as it's not a priority.

@teohhanhui teohhanhui added the bug label Apr 3, 2017
@soyuka
soyuka force-pushed the hotfix/filter-eager-loading-FETCH-EAGER branch from 84f5d7f to abba678 Compare April 3, 2017 11:50
@soyuka

soyuka commented Apr 5, 2017

Copy link
Copy Markdown
Member Author

Feel free to merge I have a note that I should refactor the shouldForceEager thing.

@dunglas
dunglas merged commit 18c1a27 into api-platform:2.0 Apr 7, 2017
@dunglas

dunglas commented Apr 7, 2017

Copy link
Copy Markdown
Member

Thank you @soyuka

@soyuka
soyuka deleted the hotfix/filter-eager-loading-FETCH-EAGER branch January 29, 2018 09:38
hoangnd25 pushed a commit to hoangnd25/core that referenced this pull request Feb 23, 2018
* Removed unneeded catch on ResourceClassNotFound

* fix FilterEagerLoading when entity has FETCH_EAGER with no force eager
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants