getkirby / ideas

This is the backlog of ideas and feature requests from the last two years. Use our new feedback platform to post your new ideas or vote on existing ideas.
https://feedback.getkirby.com
20 stars 0 forks source link

add optional() helper #587

Open bnomei opened 4 years ago

bnomei commented 4 years ago

laravel has it and could be useful in kirby as well. https://laravel.com/docs/7.x/helpers#method-optional

example in kirby

echo optional($page->image()->first())->html();

// instead of 

if ($image = $page->image()->first()) {
    echo $image->html();
}

// but maybe as fieldMethod would be more kirby style

echo $page->image()->first()->optional()->html();

not sure how its implemented in laravel and if it can be used for field methods but i would still like to see the helper in general.

afbora commented 4 years ago

+100 i also need it very often in Kirby 👍

afbora commented 4 years ago

@bnomei But it will not work. In Kirby, we access the properties by method. And if the method returns null it will fail.

For example; if $page->image() returns null, fail will null access as object.

It could be like with direct access to property, i guess it's not possible either

echo optional($page->image->first)->html();
bnomei commented 4 years ago

@afbora i think thats the point of the helper...

The optional function accepts any argument and allows you to access properties or call methods on that object. If the given object is null, properties and methods will return null instead of causing an error

bastianallgeier commented 4 years ago

If I don't miss anything I think it's pretty easy to implement.

class NullObject 
{
  public function __get() 
  {
    return null;
  }

  public function __call() 
  {
    return null;
  }

  public function __toString()
  {
      return '';
  }
}

function optional($object) 
{
    return $object ?? new NullObject();
}
bastianallgeier commented 4 years ago

I don't think the field method is possible though

afbora commented 4 years ago

@bnomei Yes it is true, but if this is the last method called, it works. Otherwise you get:

Call to a member function first() on null

Works only $page is not null:

echo optional($page->text())->excerpt();

Will not work if page has not an image:

echo optional($page->image()->first())->url();

Sent to the optional function after the parameter run.

afbora commented 4 years ago

We are experiencing the same issue for the e() helper.

https://getkirby.com/docs/reference/templates/helpers/e

afbora commented 4 years ago

I'm not sure, but maybe it can be resolved by creating an ->optional() field method. But with slightly different syntax:

$page->image()->optional('first.html');
nilshoerrmann commented 4 years ago

Thinking of the field method, could you swap positions? Like $page->optional()->image()->other()->methods()?

afbora commented 4 years ago

Then each field method has to provide null control.

If one of them returns null, throw an exception: Call to a member function xxx() on null

$page->optional()->image()->other()->methods();
                   ^^^^^^ think image() is null

I don't know if it is possible but maybe if the NullObject returns always as fallback like the example of @bastianallgeier by refactoring the __call() method 🤔

bastianallgeier commented 4 years ago

Hmmm, that's interesting. This would be a great solution if we could wait until the last method in the chain is called and then react on that method call. But as far as I know that's not possible in PHP.

nilshoerrmann commented 4 years ago

Thinking out loud: each method knows about the page context, doesn't it? So couldn't the optional() method set a flag on the page object that all methods are in "optional mode". And if image() is called on that page and it's null we are still on the site object that handles this call. So the page knows it's optional, it knows the chain is broken, so couldn't is set another flag like $skip telling all subsequent methods to exit early, doing nothing? It's the page again handling these method calls, isn't it?

afbora commented 4 years ago

I've few tests and possible with fake field that class OptionalField extends Field and seems works as expected. But only for page custom fields/methods (should be undefined methods). Because calling image() method with magic __call() method is not possible for $page->image().

I'm sure @lukasbestle has an idea for this.

lukasbestle commented 4 years ago

I have not tested this with real code yet, but I think I know how to solve this in PHP using a proxy object. The syntax would look like this:

optional($page)->images()->first()->other()->methods()->resolve();

and/or:

$page->optional()->images()->first()->other()->methods()->resolve();

What the helper and method would do is the following:

There is indeed no way to do this without a final method call that converts the proxy object back to the underlying value. So the ->resolve() call is required (but we could also add a __toString() method so ->resolve() wouldn't need to be used when a value is printed).

Do you think this would be acceptable? If so, I can prepare a PR.

afbora commented 4 years ago

@lukasbestle I liked the proxy design pattern idea. Can't we do what the resolve method does with the magic methods inside the proxy object? 🤔 then it would be great.

lukasbestle commented 4 years ago

The magic __call() method will be used to proxy method calls to the underlying object (e.g. $page). There needs to be a way to tell the proxy object to stop proxying calls to the underlying object, which is what ->resolve() does in my example.

We can use the magic __toString() method to automatically resolve the return value if the proxy object is used as a string (which will be the sign that no further method calls will be made), but in all other circumstances the proxy object needs to be told that explicitly. I don't think we can use other magic methods to make that any easier.

afbora commented 4 years ago

So can we use the resolve method as a fallback?

$page->optional()->images()->first()->url()->resolve(asset('images/placeholder.png'));
lukasbestle commented 4 years ago

Yes, that would work!

bnomei commented 4 years ago

to avoid introducing a new method called resolve...

there is a fallback field method called or() (https://getkirby.com/docs/reference/templates/field-methods/or). maybe we could make it a requirement to end a call to field method optional() with a call to field method or() (even if its empty)?

$page->optional()->images()->first()->url()->or(asset('images/placeholder.png'));
$page->optional()->images()->first()->url()->or(); // => null
lukasbestle commented 4 years ago

I like the simplicity of this!

To make sure we don't get each other wrong: When calling or(), you wouldn't actually call the field method with that name but a method directly on the proxy object. The or() naming would be great for consistency, but otherwise there is no technical difference between it and another name (like resolve()). We wouldn't reuse the code if that's what you were saying.

But in any case, I like the simple name (much better than ->resolve()). The only potential problem would be that it wouldn't be possible to call a method directly on the underlying object that is called or(). If you call ->or() on the proxy object, you will always end the chain. But we will have that issue with any name (be it resolve(), or() or something completely different).

Another alternative would be to provide a helper that would need to be used to end the chain if string conversion with the magic __toString() method isn't an option:

$optional = $page->optional()->images()->first()->url();

resolve($optional, 'default value');
or($optional, 'default value');

Advantage: No method name is blocked. Disadvantage: Much less convenient.

bnomei commented 4 years ago

i would be fine with just the helper... the main selling point is that url does not throw a null exception when being used on a first() that returned null, right? having a resolving field method is a bonus not a must for me.

$url = optional($page->optional()->images()->first()->url(), site()->url());

maybe we can make both params of optional also accept closures not just a proxy object? it would be more in line with the e()/r() helper then (does accept anything).

$url = optional(
    option('myplugin.callback'), // from plugin options
    function () use ($myvar) { // or plain php
        return $myvar;
    }
);
lukasbestle commented 4 years ago

having a resolving field method is a bonus not a must for me.

What do you mean with "resolving field method"?

maybe we can make both params of optional also accept closures not just a proxy object? it would be more in line with the e()/r() helper then (does accept anything).

Hm, now that I think about it: The existing e() and r() helpers could be used for this and detect automatically whether they get passed a proxy object. Then we don't even need any additional helper.

bnomei commented 4 years ago

What do you mean with "resolving field method"?

using optional($page->whatever()->nullHere()->chain(), 'fallback') is good enough for me. i do not need $page->optional()->whatever()->nullHere()->chain()->resolve('fallback').

bnomei commented 4 years ago

e()/r() helpers both take a condition, right? if we reuse them the param order would not match. https://getkirby.com/docs/reference/templates/helpers/r

r($condition, $a, $b);
optional($a, $b);
r($a, $b); // would evaluate $a as condition

edited: ah... you meant using $condition as $a and $a as $b if $condiction is a proxy.

lukasbestle commented 4 years ago

using optional($page->whatever()->nullHere()->chain(), 'fallback') is good enough for me.

Unfortunately this syntax won't work. We need some way (helper or page method) to initiate the proxy, so it would need to be one of these:

optional($page->optional()->whatever()->nullHere()->chain(), 'fallback');

optional(optional($page->whatever()->nullHere()->chain()), 'fallback');

edited: ah... you meant using $condition as $a and $a as $b if $condiction is a proxy.

Yes, something along these lines. We would have to see if this is viable, but we can only tell if we actually implement it.