rikbruil / Doctrine-Specification

Doctrine Specification pattern for building queries dynamically and with re-usable classes for composition.
MIT License
50 stars 7 forks source link

DQL alias required when using Embeddables in a Specification? #12

Open holtkamp opened 8 years ago

holtkamp commented 8 years ago

For some Entities we use Embeddables. It seems these Embeddables are not respected when used in a Specification? More specifically, the DQL alias e is explicitly required.

For example for an Embeddable TimeIntervalwith two properties begin and end of type DateTime this does not work:

$specification = new Specification([
    new AndX(
        new LessThanOrEquals('interval.begin', new \DateTime()),
        new GreaterThanOrEquals('interval.end', new \DateTime())
    )
]);

But this does work (note in this case the property is prefixed with the DQL alias):

$specification = new Specification([
    new AndX(
        new LessThanOrEquals('e.interval.begin', new \DateTime()),
        new GreaterThanOrEquals('e.interval.end', new \DateTime())
    )
]);

Is this behavior intended? For other properties, the DQL alias seems not required.

rikbruil commented 8 years ago

I must admit I have not used Embeddables much yet.

I will try to find out why the e alias is required in this case.

rikbruil commented 8 years ago

I believe the following line might be causing the issue you encounter: https://github.com/rikbruil/Doctrine-Specification/blob/master/src/AbstractSpecification.php#L49

There I check if an alias was provided, by checking for the existence of a . in the $value. If it was provided, I assume we don't want to prefix it with the default alias. In your not working example the code assumes interval is an alias and begin and start are the properties, while in fact interval is a property containing the embeddable.

This was probably written before I knew about the existence of Embeddables (the DQL example from the documentation also uses 2 dots for checking embeddable values).

I will try to figure out how to fix this.

ghost commented 7 years ago

Maybe it is better to remove the distinction between $field and $dqlAlias and just let the user pass whatever $dqlAlias, $field or $dqlAlias.$field he/she wants?

I have a case where I would like to have the first option. I want to make sure that the result is of a particular type, not just one field. In a table where two related types of entities are stored, Message and PrivateMessage, I want to select just the private ones. The resulting DQL should look like:

SELECT e FROM Message e WHERE e INSTANCE OF PrivateMessage

At the moment this is not possible, because $field is required. Passing an empty string would create e.

If you like that, I can try to make a PR.

holtkamp commented 7 years ago

Maybe it is better to remove the distinction between $field and $dqlAlias and just let the user pass whatever $dqlAlias, $field or $dqlAlias.$field he/she wants?

mm, that is an option indeed which makes it more flexible. But it probably means a breaking change. Currently users only provide the fieldname which will get prefixed automatically. In the suggested situation those users should provide alias.fieldname, right?

Currently typical usage would result in DQL like:

SELECT e FROM Message e WHERE e.fieldName = X;

In the suggested situation this will become:

SELECT e FROM Message e WHERE fieldName = X;

Which will result in an error:

[Semantical Error] line 0, col 53 near 'fieldName': Error: 'fieldName' is not defined. in ./vendor/doctrine/orm/lib/Doctrine/ORM/Query/Parser.php(483)

So this would mean a new major version, right?

Probably SpecificationRepository::$dqlAlias should be made protected, so Repositories that extend it can refer to it / change it when required...

holtkamp commented 7 years ago

Any progress on this? Or "snoozing"?

rikbruil commented 7 years ago

I will take a fresh look at this issue, I've been kind of busy lately unfortunately.

We could expose the $dqlAlias property, and I agree that if we change the behavior of these fields/aliases completely it should be a major version bump.

I can probably do some testing later tonight.