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

3.0 Cell::__toString() must not throw an exception #3664

Closed
k-halaburda opened this issue Jun 6, 2014 · 7 comments
Closed

3.0 Cell::__toString() must not throw an exception #3664

k-halaburda opened this issue Jun 6, 2014 · 7 comments
Assignees
Milestone

Comments

@k-halaburda
Copy link
Contributor

What i did:

in the console: /cake bake cell Sidebar
in my layout cell('Sidebar::nonexistentmethod');?>

What happened:

Warning (2): call_user_func_array() expects parameter 1 to be a valid callback, class 'App\View\Cell\SidebarCell' does not have a method 'categories' [ROOT\vendor\cakephp\cakephp\src\View\CellTrait.php, line 83]

and then:

Fatal Error
Error: Method App\View\Cell\SidebarCell::__toString() must not throw an exception

I think a fatal here is not intentional and dont really tell whats going on, warning should do the job. Php.net says You cannot throw an exception from within a __toString() method. Doing so will result in a fatal error.
http://www.php.net/manual/en/language.oop5.magic.php#object.tostring

Solution:

In View/Cell.php

public function __toString() {
try{
return $this->render();
catch(Exception $ex){
return '';
}
}

@markstory markstory added this to the 3.0.0 milestone Jun 8, 2014
@markstory
Copy link
Member

I don't think swallowing errors is a good idea. It might be better to return the exception message instead.

@ADmad
Copy link
Member

ADmad commented Jun 8, 2014

..class 'App\View\Cell\SidebarCell' does not have a method 'categories'..
Such an error is in fact preferable. It's tell you exactly what's wrong and allows you to easily fix the problem.

@markstory
Copy link
Member

Agreed, I think there are a few problems here:

  • CellTrait::cell() does not check method names. We could skip the entire __toString() issue here if an exception was raised earlier. Adding method checks and raising exceptions will solve this.
  • Cell::__toString() does not handle any errors in the view rendering well. Catching errors that occurred during rendering, and outputting the error string will solve this.

Does the above sound reasonable?

@markstory markstory self-assigned this Jun 8, 2014
@k-halaburda
Copy link
Contributor Author

I consider cells something similar to elements with its own logic (witch by the way are really great). I guess nonexistent cell methods should be threaten like non existend element files - people get used to that IMO

@josegonzalez
Copy link
Member

I would prefer that we throw an exception instead of pretending the file doesn't exist. In fact, I'd say not throwing an exception for a non-existent element is a bug, given that this is certainly not what the user wanted.

@markstory
Copy link
Member

Missing elements do trigger errors. I'll update element() to use an exception along with these changes.

markstory added a commit that referenced this issue Jun 8, 2014
Use reflection to capture missing cell methods and raise proper errors
on them.

Refs #3664
markstory added a commit that referenced this issue Jun 8, 2014
Because __toString methods cannot emit exceptions, we render a string
error. This gives the developer some feedback about what is broken.

Refs #3664
@markstory
Copy link
Member

Closing as #3676 is now open.

markstory added a commit that referenced this issue Jun 8, 2014
Use reflection to capture missing cell methods and raise proper errors
on them.

Refs #3664
markstory added a commit that referenced this issue Jun 8, 2014
Because __toString methods cannot emit exceptions, we render a string
error. This gives the developer some feedback about what is broken.

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

No branches or pull requests

4 participants