Fix silencing errors in extension handling. #12586

dereuromark · 2018-09-23T13:50:49Z

This fixes most of the issues from #12585

It does however not fix the silencing of non configured extensions.
So if you didnt whitelist ical and then just accessed /controller/action.ical it would just silently serve the content of /controller/action without any indication of problems. One would except a 404 here, though.
The reason for this last issue seems to be that in such a case routing just ignores the ext, and sets it to null for going into the controller. That's bad.

stickler-ci · 2018-09-23T13:50:58Z

src/Http/Response.php

+     *
+     * This is needed for RequestHandlerComponent and recognition of types.
+     *
+     * @param string $type


Missing parameter comment

stickler-ci · 2018-09-23T13:50:58Z

src/Http/Response.php

+     * This is needed for RequestHandlerComponent and recognition of types.
+     *
+     * @param string $type
+     * @param string|array $definition


Missing parameter comment

ADmad · 2018-09-23T14:04:04Z

src/Http/Response.php

+     * @param string|array $definition Definition of the content type.
+     * @return void
+     */
+    public function setType($type, $definition)


Why do we need setType() when there is withType() ?

See the ticket referenced. The type() deprecation lost a way to set new type definitions.
As such RequestHandler would just not be able to serve those anymore without calling

$this->response->setType('ical', 'my/ical');

etc
This has nothing to do with withType() response modification. This is only for setting/adding/replacing a (new) type into the map.

Okay, in that case addType() would be a more intuitive name since it's adds/maps a new type, doesn't actually set type for the response.

I thought about it. add() kind of implies that you could also add a type into existing mappings.
But that is not the case, this replaces existing ones completely. As such I thought set() would be more intuitive and correct.

I don't think the fact that addType() can overwrite existing mapping will cause confusion to anymore. Someone requiring to overwrite existing type mapping is unlikely anyway. mapType() would have been the ideal name but method with that name already exists for another purpose 🙁.

How about the name setTypeMap() ?

Or setTypeMapDefinition().
Modified, also added tests for it.

ADmad · 2018-09-23T14:13:54Z

src/Http/Response.php

+     * This is needed for RequestHandlerComponent and recognition of types.
+     *
+     * @param string $type Content type.
+     * @param string|array $definition Definition of the content type.


$mimeType/$contentType would be a more intuitive variable name.

markstory · 2018-09-24T00:58:12Z

Always raising a 404 on unknown 'extensions' also means that an application can never have . in the last path segment as any value after the . would be treated as an extension. I suspect this is the reason for the current behavior.

dereuromark · 2018-09-24T01:24:16Z

OK. Then this current change is still valid, as I didnt touch the non-configured ones.

But IMO there shouldn't be any such . in there, as it changes the meaning - and currently that is ignored so it is not consistent.
For me, accessing /action.foo and /action should trigger two different things unless explictly configured otherwise, right now it is the same.

ADmad · 2018-09-24T07:21:27Z

Tests should be added to give a better idea of how exactly this change impacts the handing of URLs with dot in last segment.

dereuromark · 2018-09-24T10:44:42Z

The existing tests also fail, how would we adjust those here?
I think they operate on a false assumption, as reality makes those behave differently.

markstory · 2018-09-25T01:23:10Z

src/Controller/Component/RequestHandlerComponent.php

@@ -594,6 +598,9 @@ public function renderAs(Controller $controller, $type, array $options = [])
        $viewClass = null;
        if ($builder->getClassName() === null) {
            $viewClass = App::className($view, 'View', 'View');
+            if ($viewClass === false) {
+                throw new RuntimeException('Configured view class can not be found: ' . $view);
+            }


Won't this break use cases where extensions are using sub-directories to render their views and not a custom view class?

The tests failures will go away if this particular change is reverted.

Should we only fail if you actually configure to a specific className? I feel like silently doing nothing here prevents this developer mistake from being visible.

Not sure how will you decide if it's a dev error. As @markstory pointed out one can just use View class to render content of any string type by relying on templates in sub directories and just set appropriate content/mime type for response instance.

markstory · 2018-09-25T01:24:22Z

For me, accessing /action.foo and /action should trigger two different things unless explictly configured otherwise, right now it is the same.

Is your statement for when an extension is enabled in routing? If foo is not a declared extension then I would expect .foo to be part of the last parameter. If that last parameter is the action then an error should be raised. That is how a clean application skeleton is working for me locally, and I don't think that should change.

ADmad · 2018-09-25T16:31:17Z

If foo is not a declared extension then I would expect .foo to be part of the last parameter. If that last parameter is the action then an error should be raised.

RequestHandler::$ext would be set to foo only if extension parsing has been enabled for foo isn't it? So I don't think the check and exception throwing added at line 336-338 should cause any problem.

dereuromark · 2018-09-26T22:08:57Z

Is it adjusted as per your feedback now?

ADmad · 2018-09-27T04:22:50Z

Shouldn't new test be added for change in RequestHandler?

dereuromark · 2018-09-27T09:12:00Z

Added a test case and also removed the deprecated initialize() call (as it didnt do anything anyway).

ADmad · 2018-09-27T09:36:50Z

tests/TestCase/Controller/Component/RequestHandlerComponentTest.php

+    public function testUnrecognizedExtensionFailure()
+    {
+        Router::extensions(['json', 'foo'], false);
+        $this->Controller->request = $this->Controller->request->withParam('_ext', 'foo');


Using setRequest()/getRequest() instead of the $request property will save use effort when doing 3.next/4.x merges :)

You want me to touch the code again? :)

dereuromark · 2018-09-27T17:07:18Z

All done.

markstory · 2018-09-28T17:54:27Z

src/Http/Response.php

+     * @param string|array $mimeType Definition of the mime type.
+     * @return void
+     */
+    public function setTypeMap($type, $mimeType)


Is this method setting a map or just a single type? The name and implementation don't appear to match.

A single one. Is setTypeToMap() or setTypeIntoMap() better?
I didnt want to addType() as it would also overwrite the existing one.

Hmm. On second thought I don't have a better name that isn't long.

setTypeMap() suggests that it sets whole map of types, not single type.

Main keyword here is map. After look at dictionary, I think it could be mapType(), associateType(), linkType().

I like mapType() but that one is already taken! To not confuse people I would stick to the method.

I also don't think setTypeMap() for setting one tpye within the map is a good name.
The best would really be mapType(), unfortunately.

How about setTypeMapped() or setMappedType()?
Or updateType()?

associateType(), isn't bad either.

garas · 2018-09-30T15:45:16Z

src/Http/Response.php

+    /**
+     * Sets a content type definition into the map.
+     *
+     * E.g.: setType('xhtml' => ['application/xhtml+xml', 'application/xhtml'])


Invalid syntax. Uses wrong function name.

dereuromark · 2018-10-01T13:15:41Z

Are you ok with this as is? Or do I have to use a different method name like updateTypeMap() etc?

ravage84 · 2018-10-03T13:48:14Z

Out of curiosity I setup a vanilla CakePHP and and tested some cases.

CakePHP Core 3.6.12
CakePHP App 3.6.5

Implemented TestController with event() and anotherIcal() , added the templates /Test/event.ctp & /Test/another_ical.ctp and added a route for /test/another.ical to TestController::anotherIcal() but used the fallback routes for everyhting else.

URL	RequestHandler active	Extensions active	Expected	Result
/test/event	Yes	-	TestController::event()	As expected
/test/event.ical	Yes	-	Not sure	The action event.ical is not defined in TestController *2
/test/another.ical	Yes	-	TestController::anotherIcal()	As expected
/test/event	Yes	ical	TestController::event()	As expected
/test/event.ical	Yes	ical	TestController::event()	As expected
/test/another.ical	Yes	ical	TestController::anotherIcal()	The action another is not defined in TestController *1
/test/event	No	-	TestController::event()	As expected
/test/event.ical	No	-	Not sure	The action event.ical is not defined in TestController *2
/test/another.ical	No	-	TestController::anotherIcal()	As expected
/test/event	No	ical	TestController::event()	As expected
/test/event.ical	No	ical	TestController::event()	As expected
/test/another.ical	No	ical	TestController::anotherIcal()	The action another is not defined in TestController *1

Routing should have precedence over extension handling
Action names with a dot, like event.ical, are invalid anyway, we could raise a not found exception with a more helpful errro message like The extension ".ical" is not handled nor is there a route configured..

dereuromark · 2018-10-03T14:37:18Z

Can u open that up as a separate ticket. This should not get pickybacked on this one.

ravage84 · 2018-10-03T16:33:20Z

@dereuromark thought it was relevant. Anyway, done so in #12624

Fix silencing errors in extension handling.

fb720c7

dereuromark added the defect label Sep 23, 2018

dereuromark added this to the 3.6.12 milestone Sep 23, 2018

stickler-ci bot reviewed Sep 23, 2018

View reviewed changes

Fix CS.

4bcd6f7

ADmad reviewed Sep 23, 2018

View reviewed changes

dereuromark added 2 commits September 23, 2018 16:22

Adjust as per review.

5262d0d

Add tests

78340f5

markstory reviewed Sep 25, 2018

View reviewed changes

Revert exception for strict class name finding.

cd063fa

Add test case.

2e95a15

ADmad reviewed Sep 27, 2018

View reviewed changes

Use setter on Controller.

4f174c1

Use getter methods instead of properties.

5e355ee

markstory reviewed Sep 28, 2018

View reviewed changes

markstory self-assigned this Sep 30, 2018

garas reviewed Sep 30, 2018

View reviewed changes

Fix up comment example.

6d72604

ADmad approved these changes Oct 1, 2018

View reviewed changes

markstory modified the milestones: 3.6.12, 3.6.13 Oct 2, 2018

ravage84 added the http label Oct 2, 2018

dereuromark merged commit c7c772b into master Oct 3, 2018

dereuromark deleted the bugfix/ext branch October 3, 2018 14:37

ravage84 mentioned this pull request Oct 3, 2018

RFC: File Extension Handling & Routing Behavior #12624

Closed

Libertie mentioned this pull request Nov 10, 2018

Extensions being ignored/silenced through RequestHandler #12585

Closed

3 tasks

ndm2 mentioned this pull request Dec 26, 2018

Fix handling of html and htm extensions. #12845

Merged

Fix silencing errors in extension handling. #12586

Fix silencing errors in extension handling. #12586

Conversation

dereuromark commented Sep 23, 2018 • edited

stickler-ci bot Sep 23, 2018

Choose a reason for hiding this comment

stickler-ci bot Sep 23, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dereuromark Sep 23, 2018 • edited

Choose a reason for hiding this comment

ADmad Sep 23, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dereuromark Sep 23, 2018 • edited

Choose a reason for hiding this comment

ADmad Sep 23, 2018 • edited

Choose a reason for hiding this comment

markstory commented Sep 24, 2018 • edited

dereuromark commented Sep 24, 2018 • edited

ADmad commented Sep 24, 2018

dereuromark commented Sep 24, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ADmad Sep 25, 2018 • edited

Choose a reason for hiding this comment

markstory commented Sep 25, 2018

ADmad commented Sep 25, 2018

dereuromark commented Sep 26, 2018

ADmad commented Sep 27, 2018

dereuromark commented Sep 27, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dereuromark commented Sep 27, 2018

Choose a reason for hiding this comment

dereuromark Sep 28, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dereuromark Oct 1, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dereuromark commented Oct 1, 2018

ravage84 commented Oct 3, 2018

dereuromark commented Oct 3, 2018

ravage84 commented Oct 3, 2018

dereuromark commented Sep 23, 2018 •

edited

dereuromark Sep 23, 2018 •

edited

ADmad Sep 23, 2018 •

edited

dereuromark Sep 23, 2018 •

edited

ADmad Sep 23, 2018 •

edited

markstory commented Sep 24, 2018 •

edited

dereuromark commented Sep 24, 2018 •

edited

ADmad Sep 25, 2018 •

edited

dereuromark Sep 28, 2018 •

edited

dereuromark Oct 1, 2018 •

edited