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

Automatically generate response from template in fragment controllers #622

Merged
merged 1 commit into from Aug 12, 2019

Conversation

@aschempp
Copy link
Contributor

commented Aug 7, 2019

This PR fixes and improves two things:

  1. The $template->getResponse() method will add cache headers for the current page, because it assumes it needs to generate the response for a page type. Therefore we should never use that method.

  2. The PR also simplifies the controller, because I no longer need to generate anything.

Due to the BC promise, we cannot change the getResponse method name. But we can weaken the response type, because existing child classes will enforce a response return, which is fine.

@leofeyer leofeyer added the defect label Aug 7, 2019

@leofeyer leofeyer added this to the 4.8 milestone Aug 7, 2019

@@ -53,5 +59,5 @@ protected function addSharedMaxAgeToResponse(Response $response, ContentModel $m
$response->setSharedMaxAge(min($min));
}
abstract protected function getResponse(Template $template, ContentModel $model, Request $request): Response;
abstract protected function getResponse(Template $template, ContentModel $model, Request $request): ?Response;

This comment has been minimized.

Copy link
@leofeyer

leofeyer Aug 10, 2019

Member

Why exactly is this necessary? Because it is a breaking change that will lead to a Declaration of MyController::getResponse() should be compatible with AbstractContentElementController::getResponse() exception, won't it?

This comment has been minimized.

Copy link
@aschempp

aschempp Aug 10, 2019

Author Contributor

It did not for me. Strengthening the requirement (: Response instead of : ?Response) is always allowed. Same as specifying a return value in your class even though the interface has none.

This comment has been minimized.

Copy link
@fritzmg

fritzmg Aug 10, 2019

Contributor

But it weakens the requirement from Response to ?Response - which does not seem to be necessary anway, since there will be a response now always?

This comment has been minimized.

Copy link
@aschempp

aschempp Aug 10, 2019

Author Contributor

No, you don‘t have to return a response anymore. If none is returned, the parent will generate one from the template.

This comment has been minimized.

Copy link
@leofeyer

leofeyer Aug 11, 2019

Member

It did not for me.

Then you did it wrong, because it definitely does not work: https://3v4l.org/i6uls

As you have explained yourself, it only works if the return type is more specific than the one of the parent class: https://3v4l.org/ogotc

This comment has been minimized.

Copy link
@aschempp

aschempp Aug 11, 2019

Author Contributor

Hehe, correct. And that's the point. Your controller extends our abstract controller. Your controller is more specific that this one, if you implemented the "old" interface 😉

This comment has been minimized.

Copy link
@leofeyer

leofeyer Aug 12, 2019

Member

You are absolutely right.

@Toflar

Toflar approved these changes Aug 12, 2019

@leofeyer leofeyer merged commit 8cb1c07 into 4.8 Aug 12, 2019

4 of 5 checks passed

Travis CI - Branch Build Failed
Details
Travis CI - Pull Request Build Passed
Details
continuous-integration/appveyor/branch AppVeyor build succeeded
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
coverage/coveralls Coverage decreased (-0.02%) to 85.794%
Details
@leofeyer

This comment has been minimized.

Copy link
Member

commented Aug 12, 2019

Thank you @aschempp.

@leofeyer leofeyer deleted the hotfix/controller-template-response branch Aug 12, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.