This repository has been archived by the owner. It is now read-only.

Improving JController::getInstance method #990

Closed
mbrowne opened this Issue Mar 15, 2012 · 2 comments

Comments

Projects
None yet
3 participants
@mbrowne

mbrowne commented Mar 15, 2012

Hi,
A few comments regarding the JController::getInstance method...

I came across this pull request:
#453

...and think it makes a lot of sense. Why was this closed exactly?

Also, why does the format variable get appended to the filename of the controller? For example, if format=raw and the controller is called MyController then it would look for mycontroller.raw.php. While this convention might make sense for views, I don't see how it's helpful for controllers...why would you want an entirely separate controller class just because you want one method that outputs JSON instead of HTML? And frequently you might want a method to be able to display either format.

At the very least it seems important to allow the user to specify the a format setting (which might be different from JRequest::getVar('format')) in the config array.

Also, I think the inclusion of $format on line 267 might be a bug:

throw new JException(JText::sprintf('JLIB_APPLICATION_ERROR_INVALID_CONTROLLER', $type, $format), 1056, E_ERROR, $type, true);

It seems to me this should be a format string to be passed to sprintf, if anything, not the format variable from the request.

I can split these into separate issues if that would be more helpful but they seemed so closely related that I thought it made sense to put them all in the same issue.

Thanks,
Matt

@elinw

This comment has been minimized.

Show comment Hide comment
@elinw

elinw May 12, 2012

Contributor

Do you want to update this in light of the new MVC commits? Also, if you think there's a clear bug why no just send that as a separate pull request? But I think we're not using JText at all at for exceptions.

Contributor

elinw commented May 12, 2012

Do you want to update this in light of the new MVC commits? Also, if you think there's a clear bug why no just send that as a separate pull request? But I think we're not using JText at all at for exceptions.

@eddieajau

This comment has been minimized.

Show comment Hide comment
@eddieajau

eddieajau Oct 9, 2012

Contributor

Closing this issue because, in future, we would want the application to take more control over which controller it loaded rather than the controllers trying to be way too smart for their own good.

Contributor

eddieajau commented Oct 9, 2012

Closing this issue because, in future, we would want the application to take more control over which controller it loaded rather than the controllers trying to be way too smart for their own good.

@eddieajau eddieajau closed this Oct 9, 2012

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.