Location via proxy:   [ UP ]  
[Report a bug]   [Manage cookies]                
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

feat(doctrine): search filters like laravel eloquent filters #6865

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

vinceAmstoutz
Copy link
Contributor

@vinceAmstoutz vinceAmstoutz commented Dec 12, 2024

Q A
Branch? main
License MIT
Doc PR Coming soon

Completes #6775.

TODO:

Filters support

  • IriSearchFilter
  • ExactSearchFilter
  • PartialSearchFilter
  • StartSearchFilter
  • EndSearchFilter

Extra work

  • ODM support

@vinceAmstoutz vinceAmstoutz force-pushed the feat-doctrine-search-filters-like-laravel-eloquent-filters branch 11 times, most recently from fc01fcd to 093aede Compare December 17, 2024 09:41
@vinceAmstoutz vinceAmstoutz marked this pull request as ready for review December 17, 2024 09:57
@vinceAmstoutz vinceAmstoutz force-pushed the feat-doctrine-search-filters-like-laravel-eloquent-filters branch 3 times, most recently from 7bfc697 to a9d3bd6 Compare December 17, 2024 15:02
src/Doctrine/Orm/Filter/ExactSearchFilter.php Outdated Show resolved Hide resolved
src/Doctrine/Orm/Filter/ExactSearchFilter.php Outdated Show resolved Hide resolved
src/Doctrine/Orm/Filter/IriSearchFilter.php Outdated Show resolved Hide resolved
src/Doctrine/Orm/Filter/IriSearchFilter.php Outdated Show resolved Hide resolved

$queryBuilder
->andWhere(\sprintf('%s.%s = :%s', $alias, $property, $parameterName))
->setParameter($parameterName, $resource->getId());
Copy link
Member

Choose a reason for hiding this comment

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

Properties should be available inside the $context['parameter']:

If the parameter has :property you should get an array of properties inside Parameter::extraProperties if not use Parameter::property and in the end the Parameter::key.

The only thing we need to do here is to check if our doctrine entity has that property/association (re-use some of the code that we already have for that).

src/Doctrine/Orm/Filter/IriSearchFilter.php Show resolved Hide resolved
src/State/Provider/IriConverterParameterProvider.php Outdated Show resolved Hide resolved
@vinceAmstoutz vinceAmstoutz force-pushed the feat-doctrine-search-filters-like-laravel-eloquent-filters branch 8 times, most recently from e3b56c8 to 55cb458 Compare December 24, 2024 10:03
@vinceAmstoutz vinceAmstoutz force-pushed the feat-doctrine-search-filters-like-laravel-eloquent-filters branch from 55cb458 to 06f47dd Compare December 24, 2024 10:14

public function getOpenApiParameters(Parameter $parameter): OpenApiParameter|array|null
{
return new OpenApiParameter(name: $parameter->getKey().'[]', in: 'query', style: 'deepObject', explode: true);
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think we should hardcode parameter value style in the filter, I would rather be able to define the default strategy to use in the configuration and fallback to the default

Suggested change
return new OpenApiParameter(name: $parameter->getKey().'[]', in: 'query', style: 'deepObject', explode: true);
return new OpenApiParameter(name: $parameter->getKey(), in: 'query');

should be suffisant for most of the cases.

Any thoughts @soyuka ?

Copy link
Member

Choose a reason for hiding this comment

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

If you define your own OpenApiParameter this will not get called AFAIK

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes it's the final step (before supporting Odm) @soyuka @aegypius

/**
* {@inheritdoc}
*/
public function getType(string $doctrineType): string
Copy link
Contributor Author

Choose a reason for hiding this comment

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

TO DO: remove

Comment on lines +39 to +45
if (
null === $value
|| !$this->isPropertyEnabled($property, $resourceClass)
|| !$this->isPropertyMapped($property, $resourceClass, true)
) {
return;
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

TODO: try to remove

Comment on lines +32 to +76
foreach ($context['filters'] as $property => $value) {
$this->filterProperty($this->denormalizePropertyName($property), $value, $queryBuilder, $queryNameGenerator, $resourceClass, $operation, $context);
}
}

public function getDescription(string $resourceClass): array
{
throw new RuntimeException('Not implemented.');
}

/**
* Determines whether the given property is enabled.
*/
protected function isPropertyEnabled(string $property, string $resourceClass): bool
{
if (null === $this->properties) {
// to ensure sanity, nested properties must still be explicitly enabled
return !$this->isPropertyNested($property, $resourceClass);
}

return \array_key_exists($property, $this->properties);
}

protected function denormalizePropertyName(string|int $property): string
{
if (!$this->nameConverter instanceof NameConverterInterface) {
return (string) $property;
}

return implode('.', array_map($this->nameConverter->denormalize(...), explode('.', (string) $property)));
}

public function hasManagerRegistry(): bool
{
return $this->managerRegistry instanceof ManagerRegistry;
}

public function getManagerRegistry(): ManagerRegistry
{
return $this->managerRegistry;
}

public function setManagerRegistry(ManagerRegistry $managerRegistry): void
{
$this->managerRegistry = $managerRegistry;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

TODO: try to remove

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.

3 participants