search

zfcampus / zf-doctrine-querybuilder

Doctrine Query Builder request Filters
BSD 3-Clause "New" or "Revised" License
32 stars 19 forks source link

Use metadata to verify order by field exists on row entity #36

Closed TomHAnderson closed 7 years ago

TomHAnderson commented 7 years ago

For the row alias only validate the field exists in the metadata.

This only checks the metadata for the 'row' alias. It would be nice to validate inner join aliases' metadata too in the future.

coveralls commented 7 years ago

Coverage Status

Coverage decreased (-0.08%) to 86.211% when pulling 3ff1715d37bf3142aa74aaf83221923d3f5a82a4 on TomHAnderson:hotfix/order-by-field-must-exist into c5c483e512020d2964e267b219a67aa4ff41f069 on zfcampus:master.

michalbundyra commented 7 years ago

@TomHAnderson Can you add test for your change please? Why just return and not throw exception?

TomHAnderson commented 7 years ago

re: why return and not throw an exception

https://github.com/zfcampus/zf-doctrine-querybuilder/blob/master/src/Filter/ORM/AbstractFilter.php#L37 This line also checks for valid fields on the 'row' entity and fails quietly.

These filters, and order by, are intended to be used by third parties and I don't believe a third party should be allowed to cause an exception. Therefore failing quietly is preferred.

coveralls commented 7 years ago

Coverage Status

Coverage increased (+0.03%) to 86.316% when pulling 4d68059689d7ddf4520cd138a39417a673380419 on TomHAnderson:hotfix/order-by-field-must-exist into c5c483e512020d2964e267b219a67aa4ff41f069 on zfcampus:master.

TomHAnderson commented 7 years ago

My argument against exceptions is poor as the same Field.php contains an exception. I would like to see this work quietly for every case. But I'm open to ideas.

TomHAnderson commented 7 years ago

Adding proper exceptions to this library would be useful. We'll catch them in DefaultORM and throw an ApiProblem. I'll continue this PR and build the exception handling.