Make reverse routes match [_method => GET] by default.

[beporter]

As discussed in #11467 (comment thread starts here), this PR adjusts reverse routing to match by default routes that explicitly declare [_method => GET]. This is best illustrated with an example:

// Create a route that explicitly only accepts GET.
$routes->get('/something', ['controller' => 'Foo', 'action' => 'bar']);

// Before this PR, not explicitly specifying `[_method => GET]` would fail with,
// `Error: A route matching "array ( 'controller' => 'Foo', 'action' => 'bar', 'plugin' => NULL, '_ext' => NULL, )" could not be found.`
Router::url(['controller' => 'Foo', 'action' => 'bar']);

// Providing an explicit `_method` succeeds both before and after the PR:
Router::url(['controller' => 'Foo', 'action' => 'bar', '_method' => 'GET']);

[beporter]

I'm looking at the two integration test failures now...

[beporter]

The IntegrationTestCaseTest changes deserve some explanation. The way the routes are ::setUp() in that test class, both a GET /get/posts/index request and a GET /posts/index request would route to [controller => Posts, action => index] via separate routes. But! a reverse routing lookup for [controller => Posts, action => index] would produce the /posts/index URL because the (earlier) GET-only route wouldn't match before and now does.

With the change in this PR, the tests that perform reverse lookups need to be tweaked to make sure they still match the expected route. Further details are in the commit's message.

[ADmad]

This change in default behaviour should target 3.next not master.

[dereuromark]

He considers this a bugfix. Is the current behavior broken somewhat, so that this warrants a change in behavior in a patch release?

[beporter]

@ADmad Does that override @dereuromark just tagging this PR for 3.6.12? This seems like a bug fix to me, not new functionality.

[beporter]

I'm fine either way-- I have a workaround in place using

Router::addUrlFilter(function ($params, $request) {
    return $params + ['_method' => 'GET'];
});

in my bootstrap.php file. Just say the word and I'll rebase on 3.next.

[dereuromark]

Was this behavior always broken/wrong? Or was this a change introduced at some point?
Only the latter would warrant it inside master for now IMO. Especially if we could document/publish a workaround until the minor lands.

[ADmad]

It might be unexpected behaviour but it's not a bugfix. You had to change other tests to account for your patch, so it's quite possible the change might cause problems in existing apps too.

[beporter]

@ADmad That was already discussed with @markstory in #11467 but admittedly we didn't reach a final conclusion. (And I would argue I had to correct tests that depended on incorrect behavior, but semantics. 🤷‍♀️ )

[beporter]

@dereuromark I've changed the target, but don't forget to change the milestone too. Thanks for the input, folx.

[markstory]

@ADmad Can you think of scenarios in your applications this would break?
I've never personally had 2 routes resolvig to the same location and one required _method and the other didn't in my applications.

I think this falls under the realm of bugfixes as its unexpected that using $routes->get() would require you to send _method as GET is the default request method in a few other places in CakePHP.

[ADmad]

@ADmad Can you think of scenarios in your applications this would break?

Not in my applications but I can't say about others. If you think this should go in a bugfix release then I am fine with it.

[beporter]

For my 2 cents:

• The workaround is straightforward and documented above already.
• If there’s any doubt, targeting the next minor release is the more conservative choice.
• Plus, that’s how the PR is already set up now.
• I have more interest in seeing the change merged (whenever) than I do in making sure the change is on the “correct” branch.
• ...and I’m eager to move on to other things. 😅