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.

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

Jump to bottom

EZP-29297: 404 error pages for hidden locations should be tagged #69

Merged
merged 5 commits into from Jun 18, 2018
Merged

EZP-29297: 404 error pages for hidden locations should be tagged #69

merged 5 commits into from Jun 18, 2018

Conversation

kmadejski
Copy link
Member

@kmadejski kmadejski commented Jun 14, 2018 •
edited by andrerom

Question Answer
JIRA issue EZP-29297
Bug yes
New feature no
Target version master (0.7)
BC breaks no
Tests pass yes/no
Doc needed no

These changes are related to: ezsystems/ezpublish-kernel#2350

@kmadejski kmadejski changed the base branch from master to 0.6 June 14, 2018 14:27
@kmadejski kmadejski mentioned this pull request Jun 14, 2018
Merged
4 tasks
@kmadejski kmadejski changed the title EZP-29297: 404 error pages are not tagged correctly [WIP] EZP-29297: 404 error pages are not tagged correctly Jun 14, 2018
@kmadejski kmadejski force-pushed the ezp_29297 branch 2 times, most recently from a4e2ef8 to e4f25df Compare June 14, 2018 14:34
andrerom
andrerom reviewed Jun 15, 2018
View reviewed changes
src/EventSubscriber/HiddenLocationExceptionSubscriber.php Outdated
$exception = $event->getException();

$response = $this->exceptionController->showAction($event->getRequest(), FlattenException::create($exception));
$this->responseTagger->tag($this->responseConfigurator, $response, $exception->getLocation());
Copy link
Contributor

@andrerom andrerom Jun 15, 2018 •
edited

Choose a reason for hiding this comment

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

please rebase this on top of #67 so you don't need to create response

Copy link
Contributor

Choose a reason for hiding this comment

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

As in this will also avoid you having to inject twig.controller.exception right?

Copy link
Member Author

Choose a reason for hiding this comment

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

Both correct. Thanks to #67 the solution has been significantly simplified.

@kmadejski kmadejski changed the title [WIP] EZP-29297: 404 error pages are not tagged correctly [WIP] EZP-29297: 404 error pages for hidden locations should be tagged Jun 15, 2018
@kmadejski kmadejski changed the base branch from 0.6 to add_twig_function June 15, 2018 11:12
@kmadejski kmadejski changed the title [WIP] EZP-29297: 404 error pages for hidden locations should be tagged EZP-29297: 404 error pages for hidden locations should be tagged Jun 15, 2018
@andrerom andrerom changed the base branch from add_twig_function to master June 16, 2018 09:11
@andrerom andrerom closed this Jun 16, 2018
@andrerom andrerom reopened this Jun 16, 2018
andrerom
andrerom approved these changes Jun 16, 2018
View reviewed changes
Copy link
Contributor

@andrerom andrerom left a comment •
edited

Choose a reason for hiding this comment

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

If it passes ;) (EDIT: It depends on kernel PR to pass)

@kmadejski
Copy link
Member Author

kmadejski commented Jun 18, 2018 •
edited

@andrerom / @adamwojs / @alongosz tests here didn't pass due to dependency on the kernel. This comes from the new spec which I added for this solution. I think we can safely skip forcing composer to load correct kernel branch here and act as if all tests passed 😉.

@adamwojs / @alongosz could you please review?

@alongosz
Copy link
Member

Control question: does the feature require new kernel or just the tests?

If it's the former one then this line needs to be changed to "ezsystems/ezpublish-kernel": "^7.2@dev", which is probably not what you want. Just asking... :)

@kmadejski
Copy link
Member Author

@alongosz this https://github.com/ezsystems/ezplatform-http-cache/pull/69/files#diff-27a7b10b0921b701f0f9be7a6081bcecR18 spec depends on HiddenLocationException introduced in: https://github.com/ezsystems/ezpublish-kernel/pull/2350/files#diff-39129cd096d79d4468524079d3dc6fcaR12

If we really want to make this tests pass, then I would need to change the line which you've mentioned to something like "dev-ezp_29297 as 6.13.2". Do you want me to change it to confirm?

alongosz
alongosz approved these changes Jun 18, 2018
View reviewed changes
Copy link
Member

@alongosz alongosz left a comment

Choose a reason for hiding this comment

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

Ok, if it's just a spec, then this is fine as it is.

@andrerom
Copy link
Contributor

@kmadejski can you try to skip this if class is missing?
phpspec/phpspec#296 (try to do it in let() perhaps)

@kmadejski kmadejski force-pushed the ezp_29297 branch 2 times, most recently from a53be6c to e6b14b6 Compare June 18, 2018 11:18
@andrerom andrerom merged commit ba7bd41 into ezsystems:master Jun 18, 2018
@kmadejski kmadejski deleted the ezp_29297 branch June 20, 2018 12:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@andrerom andrerom andrerom approved these changes

@alongosz alongosz alongosz approved these changes

@adamwojs adamwojs Awaiting requested review from adamwojs

Assignees
No one assigned
Labels
None yet
Milestone
No milestone
3 participants
@kmadejski @alongosz @andrerom