Skip to content
This repository has been archived by the owner on Jan 8, 2020. It is now read-only.

Conversation

fabiocarneiro
Copy link
Contributor

I want those methods to be moved from the AbstractActionController to AbstractController since they're important even if you want to dispatch directly instead of having many actions (which is what i'm doing).

Otherwise, i'll have to create my own extension of AbstractController or a trait and use it in all my project controllers, which would make necessary to have a "base" module.

The tests will still pass because the methods will be still be available in AbstractActionController, since it is an extension of AbstractController. I chose not to move the notFoundAction since it is an action and should stay in AbstractActionController.

With this update, we'll be able to:

@var MvcEvent $e
$e->setResponse($this->createHttpNotFoundModel($this->response));

@fabiocarneiro fabiocarneiro changed the title Move error generation method to abstract controller Move error generation methods to abstract controller Dec 3, 2014
@Ocramius
Copy link
Member

Ocramius commented Dec 3, 2014

These could actually be moved to controller plugins... Not sure if it represents a BC break though.

@fabiocarneiro fabiocarneiro changed the title Move error generation methods to abstract controller Move error view model generation methods to abstract controller Dec 3, 2014
@danizord
Copy link
Contributor

danizord commented Dec 3, 2014

We can move implementation to controller plugins and keep the methods as proxies for BC 👍

@Ocramius
Copy link
Member

Ocramius commented Dec 3, 2014

Good hint, @danizord!

@fabiocarneiro
Copy link
Contributor Author

Should we keep them as proxies in AbstractActionController or AbstractController? I guess they should still be moved since it makes much more sense to have them available in AbstractController implementations.

Going further, if the method will be there, does it really make sense to have it as a controller plugin? since it can't be removed from AbstractActionController anyway?

@Ocramius
Copy link
Member

Ocramius commented Dec 3, 2014

I would keep the change as small as possible. Don't move the methods, just make them proxy through to __call

@fabiocarneiro
Copy link
Contributor Author

But does it make sense even to move them to separate files? the methods will still be there anyway... y not moving them and forget about the plugins? what advantage would we have by moving them to plugins instead of keeping the methods there, since we can't remove them because of BC?

@Ocramius Also, do you have any code sample for that proxy behavior?

@Ocramius
Copy link
Member

Ocramius commented Dec 3, 2014

what advantage would we have by moving them to plugins instead of keeping the methods there, since we can't remove them because of BC?

The point is that you can override all of them in one shot.

Utility methods in controllers provide great additional flexibility for RAD.

We can't remove them as people may rely on parent::createHttpNotFoundModel(), but moving them and introducing new methods in classes inheriting from AbstractController is also an issue.

@Martin-P
Copy link
Contributor

Martin-P commented Dec 4, 2014

Could you please update the list of plugins defined in Zend\Mvc\Controller\AbstractController for IDE autocomplete? See: Zend\Mvc\Controller\AbstractController lines 26 - 43. Thanks.

$this->assertSame("Page not found", $model->getResult());
$this->assertSame(1, $model->getErrorLevel());
}
}
Copy link
Member

Choose a reason for hiding this comment

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

EOF EOL

@danizord
Copy link
Contributor

danizord commented Dec 4, 2014

👍

@Pittiplatsch
Copy link

@Ocramius:

We can't remove them as people may rely on parent::createHttpNotFoundModel()

Accessing parent::createHttpNotFoundModel() via __call() DOES work: http://3v4l.org/Cketp

@Ocramius
Copy link
Member

Ocramius commented Dec 5, 2014

@Ocramius
Copy link
Member

Ocramius commented Dec 5, 2014

@Pittiplatsch I'm just worried about subclasses overriding the method. We can remove those methods later on

@Ocramius Ocramius added this to the 2.4.0 milestone Dec 5, 2014
@Pittiplatsch
Copy link

@Ocramius Sry, I don't exactly understand the problem. Overriding is exactly what I did in my 3v4l, isn't it?

@Ocramius
Copy link
Member

Ocramius commented Dec 5, 2014

Yes, but you can't expect the plugin manager to be always the same :-) Anyway, I'm just being very defensive here.

@Ocramius Ocramius self-assigned this Dec 5, 2014
@Ocramius
Copy link
Member

Ocramius commented Dec 5, 2014

Note: should add @deprecated notices to those methods on merge, indicating that they are going to be moved to view helpers and not being part of the controller API anymore.

Ocramius added a commit that referenced this pull request Dec 5, 2014
…otFoundModel()` and `AbstractController#createConsoleNotFoundModel()` (to be removed in 2.5)
Ocramius added a commit that referenced this pull request Dec 5, 2014
…Model()` and `AbstractController#createConsoleNotFoundModel()`
Ocramius added a commit that referenced this pull request Dec 5, 2014
Ocramius added a commit that referenced this pull request Dec 5, 2014
…r plugin to be interacting on it as a unit test
Ocramius added a commit that referenced this pull request Dec 5, 2014
Ocramius added a commit that referenced this pull request Dec 5, 2014
Ocramius added a commit that referenced this pull request Dec 5, 2014
Ocramius added a commit that referenced this pull request Dec 5, 2014
Ocramius added a commit that referenced this pull request Dec 5, 2014
Ocramius added a commit that referenced this pull request Dec 5, 2014
Ocramius added a commit that referenced this pull request Dec 5, 2014
Ocramius added a commit that referenced this pull request Dec 5, 2014
Ocramius added a commit that referenced this pull request Dec 5, 2014
Ocramius added a commit that referenced this pull request Dec 5, 2014
Ocramius added a commit that referenced this pull request Dec 5, 2014
Ocramius added a commit that referenced this pull request Dec 5, 2014
@Ocramius
Copy link
Member

Ocramius commented Dec 5, 2014

@fabiocarneiro I manually merged this PR after applying some cleanups.

develop: 2e54284

@Ocramius Ocramius closed this Dec 5, 2014
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants