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.

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

Ask target entity for its identifier instead of hardcoding to id #1810

Closed
wants to merge 3 commits into from

Conversation

Toflar
Copy link
Contributor

@Toflar Toflar commented Apr 4, 2018

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

I have absolutely no clue if this is a correct fix but I wanted to open a PR here to have someone point me into the right direction. The problem is that the SearchFilter does an INNER JOIN for relations and adds WHERE $associationAlias.id hardcoded to the search query. However, my target entity does not have id as its identifier. I named the column uuid.

@dunglas dunglas requested a review from soyuka April 4, 2018 08:02
@@ -272,7 +272,8 @@ protected function filterProperty(string $property, $value, QueryBuilder $queryB

if ($metadata->isCollectionValuedAssociation($association)) {
$associationAlias = QueryBuilderHelper::addJoinOnce($queryBuilder, $queryNameGenerator, $alias, $association);
$associationField = 'id';
$targetEntityMetaData = $this->getClassMetadata($metadata->getAssociationMapping($association)['targetEntity']);
$associationField = $targetEntityMetaData->getIdentifier()[0];
Copy link
Member

Choose a reason for hiding this comment

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

it'll not work with mutliple identifiers but I guess it's better then before haha.

I'm 👍 for the change

@soyuka
Copy link
Member

soyuka commented Apr 4, 2018

hardcoded id's 😢 (ex: #1741)

@Toflar
Copy link
Contributor Author

Toflar commented Apr 4, 2018

Just found another issue. Also the IRI conversion was hardcoded to id, instead of fetching from the $associationField.

*
* @return mixed
*/
protected function getIdFromValue(string $value)
protected function getIdFromValue(string $value, string $identifier = 'id')
Copy link
Member

Choose a reason for hiding this comment

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

Unfortunately, it's a BC break http://symfony.com/doc/current/contributing/code/bc.html#changing-classes
You'll have to use func_get_args and throw a deprecation.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, I wasn't aware that this class is meant to be extended :-) I could also add a new getIdentifierFromValue() and deprecate the old one?

Copy link
Member

Choose a reason for hiding this comment

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

Yes

@Simperfit
Copy link
Contributor

Could you please check why the tests are failling ?

@Toflar
Copy link
Contributor Author

Toflar commented Apr 6, 2018

Oh, I missed them, sure!

@Toflar
Copy link
Contributor Author

Toflar commented Apr 6, 2018

Hmm, no idea what's going on there, really. I'm not so much into the search filter, I just noticed it's hardcoded to the id there.

@Toflar
Copy link
Contributor Author

Toflar commented Apr 18, 2018

@Simperfit is #1844 a follow-up to this?

@Toflar
Copy link
Contributor Author

Toflar commented May 3, 2018

I'm closing this because #1844 is the right approach.

@Toflar Toflar closed this May 3, 2018
@Toflar Toflar deleted the fix-search-filter branch May 9, 2018 07:14
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.

None yet

4 participants