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

Removed Router::parseExtensions(). #3406

Merged
merged 2 commits into from
Apr 29, 2014
Merged

Conversation

ADmad
Copy link
Member

@ADmad ADmad commented Apr 28, 2014

Just use Router::setExtensions() instead. setExtensions() now also takes
a single extension as string for first param.

Just use Router::setExtensions() instead. setExtensions() now also takes
a single extension as string for first param.
@lorenzo
Copy link
Member

lorenzo commented Apr 28, 2014

I don't like the name "setExtensions", I would rather keep "parseExtensions" and make it behave differently

@ADmad
Copy link
Member Author

ADmad commented Apr 28, 2014

I would be happy to just rename setExtensions() to parseExtensions() in the PR.

@ADmad
Copy link
Member Author

ADmad commented Apr 28, 2014

There's also Route::setExtensions(), rename that too?

@ADmad
Copy link
Member Author

ADmad commented Apr 28, 2014

Route::setExtensions() doesn't exist in 2.x so renaming it shouldn't be a problem.

@jippi
Copy link
Contributor

jippi commented Apr 28, 2014

👍 for @lorenzo suggestion :)

Also renamed Route::setExtensions() to Route::parseExtensions() for consistency.
@dereuromark
Copy link
Member

👍 Good idea!

@ceeram
Copy link
Contributor

ceeram commented Apr 28, 2014

👍 for merging parseExtensions and setExtensions, this wasn't possible in
2.x without breaking BC, hence setExtensions was added.

2014-04-28 12:23 GMT+02:00 Mark notifications@github.com:

[image: 👍] Good idea!


Reply to this email directly or view it on GitHubhttps://github.com//pull/3406#issuecomment-41543080
.

@@ -128,7 +128,7 @@ public function __construct($template, $defaults = [], array $options = []) {
* @param array $extensions The extensions to set.
* @return void
*/
public function setExtensions(array $extensions) {
public function parseExtensions(array $extensions) {
Copy link
Member

Choose a reason for hiding this comment

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

I prefer the old var args interface personally.

Copy link
Member

Choose a reason for hiding this comment

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

Would be kind of a snowflake, we don't use var args much in the framework, do we?

Copy link
Member Author

Choose a reason for hiding this comment

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

Keep in mind this is a method of Route class not Router class which generally won't be needed to be called directly.

Copy link
Member

Choose a reason for hiding this comment

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

Bah, I meant to comment on the Router method. Sorry.

@markstory markstory added this to the 3.0.0 milestone Apr 28, 2014
@markstory
Copy link
Member

This set of changes is related to #3399

@ADmad
Copy link
Member Author

ADmad commented Apr 29, 2014

Merging since there are no more objections. Will update docs too.

ADmad added a commit that referenced this pull request Apr 29, 2014
Removed Router::parseExtensions().
@ADmad ADmad merged commit ec5aa32 into cakephp:3.0 Apr 29, 2014
@ADmad ADmad deleted the 3.0-router-extensions branch April 29, 2014 04:38
@markstory
Copy link
Member

Might be good to have an upgrade shell pattern for this too.

@markstory
Copy link
Member

Thanks for making the upgrade shell issue :)

@ADmad
Copy link
Member Author

ADmad commented Apr 29, 2014

That's the least i could do 😛

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants