Actions

[theofidry]

Q	A
Bug fix?	no
New feature?	yes
BC breaks?	no
Deprecations?	no
Tests pass?	no
Fixed tickets	none
License	MIT
Doc PR	not required

Rework of the Actions to get rid of ActionUtilTrait.

Rationale: This trait contains two methods: one which decorates the Item provider to throw an HTTP exception, and the other one to extract attributes. The issues are:

• The trait is used in every action even though both methods are not.
• As the actions are final, if one which to implement a custom action, they will have little choice but to use this trait.

A consequence of the second point is that they will have to dig into the code to understand the nature of this array returned by ::extractAttributes().

Changes proposed by this PR:

• Have a proper ItemDataProvider for the HTTP context (throw a NotFoundHttpException exception)
• A class AttributesBag containing the properties required by the actions' providers, which provides a solid API instead of an array of elements
• A RequestAttributesExtractor and its interface for extracting the request attributes (previously the array return by the trait ::extractAttributes()
• A refactor of the actions to make use of the new elements above (resulting in getting rid of the trait)

Also as there was other element with an HTTP context besides the Actions, I moved Actions and those new elements under an Http namespace.

Elements to review besides those changes:

• Bound to HttpFoundation: as v2 is targeting a wilder audience, it would be nice if we could get rid of this HttpFoundation dependence and be PSR-7 compliant and simply have a bridge for HttpFoundation
• Needs more unit tests (easy to do), but blocked by Upgrade to reflection-docblock 3 phpspec/prophecy#263

[dunglas]

I've not the time to review your PR right now but HttpFoundation is used everywhere in API Platform for now (actions, Doctrine filters, events...). It's a hard dependency and it will be difficult to remove it. Having support for PSR-7 would be nice but most of our target audience (Symfony, Laravel, Durpal, phpBB...) use HttpFoundation and using the PSR-7 Bridge would be bad for performance.

IMO, except if you find a nice solution to be able to use both PSR-7 and HttpFoundation everywhere, it's better to keep HttpFoundation as a required dependency and encourage users to use the PSR-7 Bridge if they want to use this standard in their projects.

I'm also to keep this trait (sorry english guys :P). Adding a service for that is overkill and will have an impact regarding performance.

[theofidry]

but most of our target audience (Symfony, Laravel, Durpal, phpBB...) use HttpFoundation

I wouldn't be as confident as you are. For one we're excluding a good number of other frameworks, and then even for such frameworks like Laravel, I got more than once problems with trying to inject HttpFoundation Request instead of the Illuminate (because in my case the HttpFoundation one was enough), but was causing a number of issue.

except if you find a nice solution to be able to use both PSR-7 and HttpFoundation everywhere

Another problem is that Request objects are one thing, but the other places where we are using them are in Symfony listeners, which not every project (ex Laravel) have. So the library can't work at this point anyway.

But let's follow this PSR-7 discussion in #474, as it entails changing more than just the actions.

[dunglas]

@theofidry can this one be closed? Replaced by #584