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

Remove property filter as string to promote expressions #102

Merged
merged 1 commit into from
Jun 29, 2021

Conversation

ogizanagi
Copy link
Member

@ogizanagi ogizanagi commented Jun 28, 2021

I suggest to remove this syntax:

$users = $contentManager->getContents(User::class, null, 'active');

in favor of expressions:

$users = $contentManager->getContents(User::class, null, '_.active');

since:

  • it's not much more complex to write

  • it allows to enable first-class support for expressions as filters, removing the need to create an Expression instance with expr/ exprOr, and expressions are way more powerful:

    -$users = $contentManager->getContents(User::class, null, expr('_.active'));
    +$users = $contentManager->getContents(User::class, null, '_.active');
  • we do not seem to use this syntax since the begining, but rather use:

    $users = $contentManager->getContents(User::class, null, ['active' => true]);

WDYT?

@ogizanagi ogizanagi added the enhancement New feature or request label Jun 28, 2021
@ogizanagi ogizanagi requested a review from Tom32i June 28, 2021 15:12
Copy link
Collaborator

@Tom32i Tom32i left a comment

Choose a reason for hiding this comment

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

Ok with me! Technically it's a BC break right?

@ogizanagi
Copy link
Member Author

ogizanagi commented Jun 29, 2021

True, as in the latest releases, we'll add a small note:

Breaking Changes ⚠️

The ContentManager::getContents() method now accepts string based expressions as filter.
As a consequence, the ability to use a property name as filter directly as a string was removed. Use an expression instead:

-$users = $contentManager->getContents(User::class, null, 'active');
+$users = $contentManager->getContents(User::class, null, '_.active');

@ogizanagi ogizanagi merged commit 2827425 into master Jun 29, 2021
@ogizanagi ogizanagi deleted the first-class-expr branch June 29, 2021 08:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants