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

Add default parameter values to method #3485

Merged
merged 1 commit into from Feb 3, 2016
Merged

Add default parameter values to method #3485

merged 1 commit into from Feb 3, 2016

Conversation

skybluesofa
Copy link
Contributor

The \Concrete\Core\Controller\Controller::get method has a signature of get($identifier=null, $defaultValue=null). This signature was causing issues on PHP7.

The \Concrete\Core\Controller\Controller::get method has a signature of get($identifier=null, $defaultValue=null). This signature was causing issues on PHP7.
@KorvinSzanto
Copy link
Member

This looks like it's probably extending the base method by accident, gotta leave it like this for backwards compatibility though.

KorvinSzanto added a commit that referenced this pull request Feb 3, 2016
Add default parameter values to Feed::get method
@KorvinSzanto KorvinSzanto merged commit fdfd7b1 into concretecms:develop Feb 3, 2016
@aembler
Copy link
Member

aembler commented Feb 3, 2016

Yeah, honestly we should change this method to something like output($identifier) instead of get(). We could leave the route the same and simply change the method it's calling, so no issue with backward compatibility.

@KorvinSzanto
Copy link
Member

@aembler I just imagine that if someone has overridden this feed controller and extended the get method, their customizations will no longer apply since the route would be looking to a different method. That said, I agree that we should change this and keep track of the backwards incompatibility.

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

Successfully merging this pull request may close these issues.

None yet

3 participants