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

2.0 appshell #85

Merged
merged 1 commit into from May 19, 2011
Merged

2.0 appshell #85

merged 1 commit into from May 19, 2011

Conversation

sitedyno
Copy link
Contributor

Load AppShell class.

This was discussed here and marked as resolved http://cakephp.lighthouseapp.com/projects/42648/tickets/1114-rfc-add-support-for-an-appshell-class but somehow the actual loading of the class got lost/left out.

@sitedyno
Copy link
Contributor Author

There is something else I was wondering about. Shell::hasMethod() does a check to see if the method being called was declared by the current class or not. I assume this is to prevent public methods of Shell being used/abused out of context. The way the check is currently you cannot call public methods of AppShell either. I'm not sure if its a bug or feature ;) Any thoughts? It could easily be refactored to disallow things in Shell, but allow things from AppShell.

@renan
Copy link
Contributor

renan commented May 19, 2011

Well, also the shells still don't extends the AppShell.
I believe that should be done as well, not sure though.

@markstory
Copy link
Member

Looks like the loading of AppShell got lost in the conversion to App::uses() over App::import(), thanks for finding and fixing that.

My thoughts were that it seemed unlikely that you'd want a callable shell method on all shells in an application. I guess its theoretically possible you would. In that case you'd have to make sure the method wasn't declared on Shell. In any case I originally wrote it with the idea that someone wouldn't need the same callable method on every shell.

@renan
Copy link
Contributor

renan commented May 19, 2011

Maybe not create a callable method on every shell, but changing the way the output is displayed or the input gathered.

lorenzo added a commit that referenced this pull request May 19, 2011
@lorenzo lorenzo merged commit 4d35d63 into cakephp:2.0 May 19, 2011
@sitedyno
Copy link
Contributor Author

Ya I don't see it as a big deal. Now that there is an AppShell, it's easy enough to overload hasMethod to get the behavior you want.

I don't think core shells need to extend AppShell. Could cause confusion and erroneous bug reports?

@markstory
Copy link
Member

Yeah, they don't currently, and I plan on keeping them that way. Letting AppShell screw with bake is not the idea behind AppShell. :)

dogmatic69 added a commit to dogmatic69/cakephp that referenced this pull request Jun 19, 2012
dogmatic69 added a commit to dogmatic69/cakephp that referenced this pull request Sep 12, 2012
markstory pushed a commit that referenced this pull request Sep 21, 2012
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

4 participants