Skip to content

Remove http_cache.invalidation deprecation and fix some remaining deprec #3276

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.

Sign up for GitHub

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

Merged
soyuka merged 4 commits into from
Apr 9, 2020
Merged

Remove http_cache.invalidation deprecation and fix some remaining deprec #3276

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
4406174
Remove http_cache.invalidation deprecation and fix some remaining deprec
dunglas Nov 19, 2019
690c28c
Fix pagination options with defaults
soyuka Mar 29, 2020
3492922
Fix order being a string
soyuka Apr 9, 2020
fbfa615
FOSUserBundle deprecates GroupableInterface used by our BaseModel
soyuka Apr 9, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion .github/workflows/ci.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2008,7 +2008,7 @@ jobs:
fail-fast: false
timeout-minutes: 20
env:
SYMFONY_DEPRECATIONS_HELPER: max[total]=5 # 5 deprecation notices from FOSUserBundle
SYMFONY_DEPRECATIONS_HELPER: max[total]=8 # 5 deprecation notices from FOSUserBundle
steps:
- name: Checkout
uses: actions/checkout@v1
Expand Down
2 changes: 1 addition & 1 deletion src/Bridge/Doctrine/MongoDbOdm/Extension/OrderExtension.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,7 @@ public function applyToCollection(Builder $aggregationBuilder, string $resourceC
$identifiers = $classMetaData->getIdentifier();
if (null !== $this->resourceMetadataFactory) {
$defaultOrder = $this->resourceMetadataFactory->create($resourceClass)->getAttribute('order');
if (null !== $defaultOrder) {
if (\is_array($defaultOrder)) {
foreach ($defaultOrder as $field => $order) {
if (\is_int($field)) {
// Default direction
Expand Down
2 changes: 1 addition & 1 deletion src/Bridge/Doctrine/Orm/Extension/OrderExtension.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,7 @@ public function applyToCollection(QueryBuilder $queryBuilder, QueryNameGenerator
$identifiers = $classMetaData->getIdentifier();
if (null !== $this->resourceMetadataFactory) {
$defaultOrder = $this->resourceMetadataFactory->create($resourceClass)->getAttribute('order');
if (null !== $defaultOrder) {
if (\is_array($defaultOrder)) {
foreach ($defaultOrder as $field => $order) {
if (\is_int($field)) {
// Default direction
Expand Down
56 changes: 37 additions & 19 deletions src/Bridge/Symfony/Bundle/DependencyInjection/ApiPlatformExtension.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -167,26 +167,26 @@ private function registerCommonConfiguration(ContainerBuilder $container, array
$container->setParameter('api_platform.eager_loading.fetch_partial', $config['eager_loading']['fetch_partial']);
$container->setParameter('api_platform.eager_loading.force_eager', $config['eager_loading']['force_eager']);
$container->setParameter('api_platform.collection.exists_parameter_name', $config['collection']['exists_parameter_name']);
$container->setParameter('api_platform.collection.order', $config['collection']['order']);
$container->setParameter('api_platform.collection.order', $config['defaults']['order'] ?? $config['collection']['order']);

@guilliamxavier guilliamxavier Mar 26, 2021 •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This change was wrong! defaults.order is supposed to be the global resource default for https://api-platform.com/docs/core/default-order/#overriding-default-order i.e. an array like {"foo":"ASC"} or ["foo", "bar"], not to be confused with collection.order that is a string "ASC" or "DESC"!

I first wrote this:

api_platform:
    defaults:
        order: [createdAt, id]

but got this error:

Argument 1 passed to ApiPlatform\Core\Bridge\Doctrine\Orm\Extension\OrderExtension::__construct() must be of the type string or null, array given

Fortunately I could work around by using the longer:

api_platform:
    defaults:
        attributes:
            order: [createdAt, id]

but still...

Can this change be reverted as a bug fix or would it be a BC break now?

@alanpoulain alanpoulain Mar 26, 2021

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

IMO it's more a bugfix since this change is wrong.

@guilliamxavier guilliamxavier Mar 29, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks, I opened #4170

$container->setParameter('api_platform.collection.order_parameter_name', $config['collection']['order_parameter_name']);
$container->setParameter('api_platform.collection.pagination.enabled', $this->isConfigEnabled($container, $config['collection']['pagination']));
$container->setParameter('api_platform.collection.pagination.partial', $config['collection']['pagination']['partial']);
$container->setParameter('api_platform.collection.pagination.client_enabled', $config['collection']['pagination']['client_enabled']);
$container->setParameter('api_platform.collection.pagination.client_items_per_page', $config['collection']['pagination']['client_items_per_page']);
$container->setParameter('api_platform.collection.pagination.client_partial', $config['collection']['pagination']['client_partial']);
$container->setParameter('api_platform.collection.pagination.items_per_page', $config['collection']['pagination']['items_per_page']);
$container->setParameter('api_platform.collection.pagination.maximum_items_per_page', $config['collection']['pagination']['maximum_items_per_page']);
$container->setParameter('api_platform.collection.pagination.page_parameter_name', $config['collection']['pagination']['page_parameter_name']);
$container->setParameter('api_platform.collection.pagination.enabled_parameter_name', $config['collection']['pagination']['enabled_parameter_name']);
$container->setParameter('api_platform.collection.pagination.items_per_page_parameter_name', $config['collection']['pagination']['items_per_page_parameter_name']);
$container->setParameter('api_platform.collection.pagination.partial_parameter_name', $config['collection']['pagination']['partial_parameter_name']);
$container->setParameter('api_platform.collection.pagination', $config['collection']['pagination']);
$container->setParameter('api_platform.http_cache.etag', $config['http_cache']['etag']);
$container->setParameter('api_platform.http_cache.max_age', $config['http_cache']['max_age']);
$container->setParameter('api_platform.http_cache.shared_max_age', $config['http_cache']['shared_max_age']);
$container->setParameter('api_platform.http_cache.vary', $config['http_cache']['vary']);
$container->setParameter('api_platform.http_cache.public', $config['http_cache']['public']);
$container->setParameter('api_platform.http_cache.invalidation.max_header_length', $config['http_cache']['invalidation']['max_header_length']);
$container->setParameter('api_platform.collection.pagination.enabled', $this->isConfigEnabled($container, $config['defaults']['pagination_enabled'] ?? $config['collection']['pagination']));
$container->setParameter('api_platform.collection.pagination.partial', $config['defaults']['pagination_partial'] ?? $config['collection']['pagination']['partial']);
$container->setParameter('api_platform.collection.pagination.client_enabled', $config['defaults']['pagination_client_enabled'] ?? $config['collection']['pagination']['client_enabled']);
$container->setParameter('api_platform.collection.pagination.client_items_per_page', $config['defaults']['pagination_client_items_per_page'] ?? $config['collection']['pagination']['client_items_per_page']);
$container->setParameter('api_platform.collection.pagination.client_partial', $config['defaults']['pagination_client_partial'] ?? $config['collection']['pagination']['client_partial']);
$container->setParameter('api_platform.collection.pagination.items_per_page', $config['defaults']['pagination_items_per_page'] ?? $config['collection']['pagination']['items_per_page']);
$container->setParameter('api_platform.collection.pagination.maximum_items_per_page', $config['defaults']['pagination_maximum_items_per_page'] ?? $config['collection']['pagination']['maximum_items_per_page']);
$container->setParameter('api_platform.collection.pagination.page_parameter_name', $config['defaults']['pagination_page_parameter_name'] ?? $config['collection']['pagination']['page_parameter_name']);
$container->setParameter('api_platform.collection.pagination.enabled_parameter_name', $config['defaults']['pagination_enabled_parameter_name'] ?? $config['collection']['pagination']['enabled_parameter_name']);
$container->setParameter('api_platform.collection.pagination.items_per_page_parameter_name', $config['defaults']['pagination_items_per_page_parameter_name'] ?? $config['collection']['pagination']['items_per_page_parameter_name']);
$container->setParameter('api_platform.collection.pagination.partial_parameter_name', $config['defaults']['pagination_partial_parameter_name'] ?? $config['collection']['pagination']['partial_parameter_name']);
$container->setParameter('api_platform.collection.pagination', $this->getPaginationDefaults($config['defaults'], $config['collection']['pagination']));
$container->setParameter('api_platform.http_cache.etag', $config['defaults']['cache_headers']['etag'] ?? $config['http_cache']['etag']);
$container->setParameter('api_platform.http_cache.max_age', $config['defaults']['cache_headers']['max_age'] ?? $config['http_cache']['max_age']);
$container->setParameter('api_platform.http_cache.shared_max_age', $config['defaults']['cache_headers']['shared_max_age'] ?? $config['http_cache']['shared_max_age']);
$container->setParameter('api_platform.http_cache.vary', $config['defaults']['cache_headers']['vary'] ?? $config['http_cache']['vary']);
$container->setParameter('api_platform.http_cache.public', $config['defaults']['cache_headers']['public'] ?? $config['http_cache']['public']);
$container->setParameter('api_platform.http_cache.invalidation.max_header_length', $config['defaults']['cache_headers']['invalidation']['max_header_length'] ?? $config['http_cache']['invalidation']['max_header_length']);

$container->setAlias('api_platform.operation_path_resolver.default', $config['default_operation_path_resolver']);
$container->setAlias('api_platform.path_segment_name_generator', $config['path_segment_name_generator']);
Expand All @@ -197,6 +197,24 @@ private function registerCommonConfiguration(ContainerBuilder $container, array
$container->setParameter('api_platform.defaults', $this->normalizeDefaults($config['defaults'] ?? []));
}

/**
* This method will be removed in 3.0 when "defaults" will be the regular configuration path for the pagination.
*/
private function getPaginationDefaults(array $defaults, array $collectionPaginationConfiguration): array
{
$paginationOptions = [];

foreach ($defaults as $key => $value) {
if (0 !== strpos($key, 'pagination_')) {
continue;
}

$paginationOptions[str_replace('pagination_', '', $key)] = $value;
}

return array_merge($collectionPaginationConfiguration, $paginationOptions);
}

private function normalizeDefaults(array $defaults): array
{
$normalizedDefaults = ['attributes' => []];
Expand Down
1 change: 0 additions & 1 deletion src/Bridge/Symfony/Bundle/DependencyInjection/Configuration.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -366,7 +366,6 @@ private function addHttpCacheSection(ArrayNodeDefinition $rootNode): void
->end()
->booleanNode('public')->defaultNull()->info('To make all responses public by default.')->end()
->arrayNode('invalidation')
->setDeprecated('The use of the `http_cache.invalidation` has been deprecated in 2.6 and will be removed in 3.0. Use `defaults.cache_headers.invalidation` instead.')
Comment thread
teohhanhui marked this conversation as resolved.
->info('Enable the tags-based cache invalidation system.')
->canBeEnabled()
->children()
Expand Down
29 changes: 14 additions & 15 deletions tests/Fixtures/app/config/config_common.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,25 +58,24 @@ api_platform:
nesting_separator: __
name_converter: 'app.name_converter'
enable_fos_user: true
collection:
order_parameter_name: 'order'
order: 'ASC'
pagination:
client_enabled: true
client_items_per_page: true
client_partial: true
items_per_page: 3
exception_to_status:
Symfony\Component\Serializer\Exception\ExceptionInterface: !php/const Symfony\Component\HttpFoundation\Response::HTTP_BAD_REQUEST
ApiPlatform\Core\Exception\InvalidArgumentException: !php/const Symfony\Component\HttpFoundation\Response::HTTP_BAD_REQUEST
ApiPlatform\Core\Exception\FilterValidationException: !php/const Symfony\Component\HttpFoundation\Response::HTTP_BAD_REQUEST
http_cache:
invalidation:
enabled: true
max_age: 60
shared_max_age: 3600
vary: ['Accept', 'Cookie']
public: true
defaults:
pagination_client_enabled: true
Comment thread
soyuka marked this conversation as resolved.
pagination_client_items_per_page: true
pagination_client_partial: true
pagination_items_per_page: 3
order: 'ASC'
cache_headers:
max_age: 60
shared_max_age: 3600
vary: ['Accept', 'Cookie']
public: true

parameters:
container.autowiring.strict_mode: true
Expand Down Expand Up @@ -144,19 +143,19 @@ services:
ApiPlatform\Core\Tests\Fixtures\TestBundle\Filter\RequiredAllowEmptyFilter:
arguments: [ '@doctrine' ]
tags: [ 'api_platform.filter' ]

ApiPlatform\Core\Tests\Fixtures\TestBundle\Filter\BoundsFilter:
arguments: [ '@doctrine' ]
tags: [ 'api_platform.filter' ]

ApiPlatform\Core\Tests\Fixtures\TestBundle\Filter\LengthFilter:
arguments: [ '@doctrine' ]
tags: [ 'api_platform.filter' ]

ApiPlatform\Core\Tests\Fixtures\TestBundle\Filter\PatternFilter:
arguments: [ '@doctrine' ]
tags: [ 'api_platform.filter' ]

ApiPlatform\Core\Tests\Fixtures\TestBundle\Controller\:
resource: '../../TestBundle/Controller'
tags: ['controller.service_arguments']
Expand Down
2 changes: 2 additions & 0 deletions tests/Metadata/Resource/Factory/AnnotationResourceMetadataFactoryTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,7 @@ public function testCreateWithDefaults()
$annotation = new ApiResource([
'itemOperations' => ['get', 'delete'],
'attributes' => [
'pagination_client_enabled' => true,
'pagination_maximum_items_per_page' => 10,
],
]);
Expand All @@ -74,6 +75,7 @@ public function testCreateWithDefaults()
$this->assertEquals('CHANGEME!', $metadata->getDescription());
$this->assertEquals(['get'], $metadata->getCollectionOperations());
$this->assertEquals(['get', 'delete'], $metadata->getItemOperations());
$this->assertTrue($metadata->getAttribute('pagination_client_enabled'));
$this->assertEquals(4, $metadata->getAttribute('pagination_items_per_page'));
$this->assertEquals(10, $metadata->getAttribute('pagination_maximum_items_per_page'));
}
Expand Down