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

Support NOT expression #10234

Merged
merged 1 commit into from Dec 28, 2022
Merged

Support NOT expression #10234

merged 1 commit into from Dec 28, 2022

Conversation

remi-san
Copy link
Contributor

@remi-san remi-san commented Nov 15, 2022

This PR aims at providing support for the NOT expression which might to be a part of doctrine/collections in the future.

It is not mergeable as it references a branch that has not been merged yet into doctrine/collections (see PR #348 on doctrine/collections) but will be updated as soon as it is. doctrine/collections 2.1.0 having been released, this PR is now complete.

Given the fact that the not expression is already supported by Doctrine\ORM\Query\Expr, it is pretty straightforward.

@greg0ire
Copy link
Member

It is not mergeable as it references a branch that has not been merged yet into doctrine/collections (see doctrine/collections#348) but will be updated as soon as it is.

Converting to draft until then 👍

@greg0ire greg0ire marked this pull request as draft November 15, 2022 19:11
@greg0ire
Copy link
Member

This should target 2.14.x since it's a new feature. Also, should docs be updated?

@remi-san
Copy link
Contributor Author

remi-san commented Nov 16, 2022

The reason I put it under 3.0.x was that I assumed forcing an update to doctrine/collection version ^2.1 was maybe a bit too much for a minor version.

But, if you tell me that's not a problem, I'll be glad to rebase my branch on 2.14.x.

@greg0ire
Copy link
Member

🤔 you're right, that won't be OK… but maybe you can make the new ORM feature optional? Meaning that if doctrine/collection is < 2.1, the feature is not enabled?

@remi-san
Copy link
Contributor Author

@greg0ire I have no idea on how to do that 😅 . If you can point me to some resource on the matter, I'll be glad to try something.

And I will update the doc, of course.

@greg0ire
Copy link
Member

A way to detect that your feature is present would be to use method_exists(CompositeExpression::class, 'not'). Based on that, you can decide to do something or throw an exception defined in https://github.com/doctrine/orm/blob/2.13.x/lib/Doctrine/ORM/Exception/NotSupported.php

@remi-san remi-san changed the base branch from 3.0.x to 2.14.x November 16, 2022 19:35
@remi-san
Copy link
Contributor Author

Ok, so I've done that. For my use case I don't think I need to throw any exception as if it's a previous version of doctrine/collections, the code won't be called as the cases can't exist.

Still waiting for the doctrine/collections v2.1.0 to be tagged before it can stop being a draft, but I think everything is ready otherwise.

I just found one relevant reference to the collections in the doc and updated it, but might have missed something.

@greg0ire
Copy link
Member

Still waiting for the doctrine/collections v2.1.0 to be tagged before it can stop being a draft, but I think everything is ready otherwise.

Let's wait for a green build before doing that… it would be a shame to release v2.1.0 only to have to release v2.1.1 immediately after in case we are missing something.

@remi-san
Copy link
Contributor Author

I just updated composer.json to use the correct version of doctrine/collections. This is not a draft anymore.

@remi-san remi-san marked this pull request as ready for review November 19, 2022 13:00
@greg0ire
Copy link
Member

Let's wait on #10239

@derrabus
Copy link
Member

What's the status of this PR?

@remi-san
Copy link
Contributor Author

What's the status of this PR?

I was waiting on the pipeline to be launched after rebasing my branch because something else broke it. But it seems in the mean time, some other commits broke it, so I've rebased it again and this should be good now.

@derrabus derrabus changed the base branch from 2.14.x to 2.15.x December 20, 2022 08:21
@derrabus derrabus merged commit c7f2a1d into doctrine:2.15.x Dec 28, 2022
@derrabus derrabus added this to the 2.15.0 milestone Dec 28, 2022
@remi-san remi-san deleted the not-expression-support branch December 28, 2022 16:54
derrabus added a commit to derrabus/orm that referenced this pull request Dec 28, 2022
* 2.15.x:
  Support of NOT expression from doctrine/collections ^2.1 (doctrine#10234)
  Fix Psalm errors with Collection 2.1.2 (doctrine#10343)
  Added warning about query cache in relation to parameters (doctrine#10276)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants