Begin Introduction of new 'sunset' API-Resource annotation in order t… #2252
Conversation
blanky0230
force-pushed
the
support_sunset_header
branch
from
3da47bd to
8664190
Compare
blanky0230
force-pushed
the
support_sunset_header
branch
from
8664190 to
f841a1a
Compare
| use ApiPlatform\Core\Util\RequestAttributesExtractor; | ||
| use Symfony\Component\HttpKernel\Event\FilterResponseEvent; | ||
|
|
||
| class AddSunsetHeaderListener |
There was a problem hiding this comment.
Can you make it final please? (sorry I forgot about that before)
There was a problem hiding this comment.
Double thinking about this, it will be even better to remove this class and move its code in RespondListener.
Sending HTTP headers like this one is the responsibility of the responder.
There was a problem hiding this comment.
Seems like a very good idea. Feels way cleaner :) I moved it to Eventlistener/RespondListener, could you please take a look at the test, I'm not 100% certain I did it right.
|
|
||
| declare(strict_types=1); | ||
|
|
||
| namespace ApiPlatform\Core\Sunset\EventListener; |
There was a problem hiding this comment.
Just wondering why not directly in Core\Eventlistener and I faill to see where is the listener declared (src/Bridge/Symfony/Bundle/Resources/config/api.xml) ?
There was a problem hiding this comment.
Good catch. Should be in core, and of course registered, thank @antograssiot
There was a problem hiding this comment.
So i moved it to EventListener\RespondListner::onKernelView(), was that what you had in mind @antograssiot ? Since I do no longer introduce a new Listener I suppose that's fine :)
…o support: sunset header
blanky0230
force-pushed
the
support_sunset_header
branch
from
f841a1a to
c55efb3
Compare
| @@ -55,6 +55,11 @@ public function onKernelView(GetResponseForControllerResultEvent $event) | |||
| } | |||
| } | |||
|
|
|||
| if ($request->attributes->has('sunset')) { | |||
There was a problem hiding this comment.
you are not using the value from the ApiResource attribute here.
I've opened a PR on your fork to show you what I had in mind/understood, and also added a Behat test.
You can get inspiration from that or use the code as is, I don't mind
There was a problem hiding this comment.
I pulled your pr in :)
Extract the sunset header from attributes
There was a problem hiding this comment.
@dunglas you might want to have another look at this PR as the code changed quite significantly since you approved it
|
Thanks! Great improvement. |
I messed up with rebasing on the old PR :/
I'm sorry if this was kinda spammy. (git rebase is a bit new to me).