whitecube / nova-page

Static pages content management for Laravel Nova
https://whitecube.github.io/nova-page
MIT License
238 stars 41 forks source link

Fail Safe Page::get() + Default Option #31

Closed MannikJ closed 5 years ago

MannikJ commented 5 years ago

We were a bit surprised to see what the Page::get() does if the the content does not exist. We had the issue that without content properly initialized the whole thing blew up, because Page::get() threw an exception. I think it would make more sense to make it more resilient and return null or a passed default value instead. The same applies for loadForRoute() as well. Also the documentation always mentions it as loadFromRoute() - that function does not exist.

raphcollective commented 5 years ago

The easiest way to solve this issue is to create a middleware LoadPageForCurrentRoute based on the one from the package and change the loadForRoute call to this:

$this->page->loadForRoute($request->route(), true, false);

The third parameter is the one we're interested in, it's $throwOnMissing, and I think you can guess what it does. If you need more detail on this function you can find it here.

Of course you'll also need to change your route configuration to use your new middleware.

That should fix your issue, with that said I agree that $throwOnMissing should have a value of false by default, I think it makes more sense.

MannikJ commented 5 years ago

Ok, shame on me for missing that. This is exactly what I mean, but this only applies for the loadForRoute/load and not Page::get, correct?

toonvandenbos commented 5 years ago

Hi @MannikJ,

That is correct. The Page::get method retrieves a value on the current page's template. In case something went wrong, it will result in null if there is no current page loaded, throw an Whitecube\NovaPage\Exceptions\ValueNotFoundException if the template was found but the attribute is not defined or return null if the the template was found AND was previously loaded with $throwOnMissing set to false.

I'm considering setting $throwOnMissing to false by default since it seems to create a lot of confusion.

toonvandenbos commented 5 years ago

After using the package on multiple projects myself, I have to agree it is indeed easier to use with silent failures.

The default value of $throwOnMissing has been set to false, meaning you can still throw the ValueNotFoundException by loading pages with the $throwOnMissing parameter set to true (or change it in your own LoadPageForCurrentRoute middleware as suggested by @raphcollective).

I did not add a default value parameter to the Page::get() method since you can easily achieve this in a more readable way using PHP's Null coalescing operator.

GarethSomers commented 5 years ago

Can I suggest the throwOnMissing parameter is controlled in config/novapage.php