Remove http_cache.invalidation deprecation and fix some remaining deprec#3276
Conversation
dunglas
commented
Nov 19, 2019
dunglas
commented
| Q | A |
|---|---|
| Bug fix? | yes |
| New feature? | no |
| BC breaks? | no |
| Deprecations? | no |
| Tickets | n/a |
| License | MIT |
| Doc PR | n/a |
soyuka
force-pushed
the
fix-deprecations
branch
4 times, most recently
from
811c1f8 to
c6310d3
Compare
| @@ -169,24 +169,24 @@ private function registerCommonConfiguration(ContainerBuilder $container, array | |||
| $container->setParameter('api_platform.collection.exists_parameter_name', $config['collection']['exists_parameter_name']); | |||
| $container->setParameter('api_platform.collection.order', $config['collection']['order']); | |||
There was a problem hiding this comment.
This needs to be changed, no?
There was a problem hiding this comment.
Indeed, not sure yet if we should deprecate these parameters?
| pagination_client_items_per_page: true | ||
| pagination_client_partial: true | ||
| pagination_items_per_page: 3 | ||
| order_parameter_name: 'order' |
There was a problem hiding this comment.
There's no such key supported in the defaults. In fact, all the _parameter_name attributes are not there.
There was a problem hiding this comment.
👍 in fact the defaults are meant to be used on the @ApiResource (IIUC) and parameter names are supposed to be globally defined ?
There was a problem hiding this comment.
Parameter names are customizable per-filter, so indeed they have no place in the resource / operation attributes, I think...
| return; | ||
| } | ||
| if (\is_string($defaultOrder)) { | ||
| $this->order = $defaultOrder; |
There was a problem hiding this comment.
This looks wrong to me.
soyuka
force-pushed
the
fix-deprecations
branch
4 times, most recently
from
f3b8f52 to
a5b0d4c
Compare
|
Note that #3472 will improve this |
| $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']); |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
IMO it's more a bugfix since this change is wrong.
5 participants
