Fix error when using Laravel's Eloquent methods calls

[vitalybaev]

Hello! I'm using pug-php via pug-laravel package and encountered error, when use Laravel's Eloquent methods in Pug:

ErrorException in HasAttributes.php line 403:
Relationship method must return an object of type Illuminate\Database\Eloquent\Relations\Relation (View: /var/contributing/php-pug-eloquent-bug-demo/resources/views/welcome.pug)

You could check out demo code here: https://github.com/vitalybaev/php-pug-eloquent-bug-demo and live result:

Normal class method: http://php-pug-bug.vitalybaev.ru/test_normal_class

Eloquent class method: http://php-pug-bug.vitalybaev.ru/test_eloquent.

I've changed the order of cheks in src/Jade/Compiler/CompilerFacade::getPropertyFromObject

All test passed.

[codecov-io]

Codecov Report

Merging #124 into master will not change coverage. The diff coverage is 100%.

@@           Coverage Diff           @@
##             master   #124   +/-   ##
=======================================
  Coverage       100%   100%           
  Complexity      958    958           
=======================================
  Files            59     59           
  Lines          2353   2351    -2     
=======================================
- Hits           2353   2351    -2

Impacted Files	Coverage Δ	Complexity Δ
src/Jade/Compiler/CompilerFacade.php	100% <100%> (ø)	36 <0> (ø)	:arrow_down:

---

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 7049ade...5f8d4b0. Read the comment docs.

[kylekatarnls]

Hi, thanks. I'm OK for ->getProp() first, but I would prefer ->prop keep precedence over ->prop(). I tried and tests still pass this way. Would it be OK for your Eloquent use?

PS: I see ternary is too confused and you change it. I'm OK with that, also note you can use the early-return pattern (means the 2 elseif can be replaced with just if in your code).

[kylekatarnls]

We will also need js-phpize to have the same behaviour: https://github.com/pug-php/js-phpize/blob/master/src/JsPhpize/Compiler/Helpers/Dot.h

js-phpize is used when you select 'languageExpression' => 'js'. In the next version, the pug engine will not handle this, it means: foo.bar will not at all be transformed, it will be read as concatenation of PHP constants. So js-phpize as expression plugin will be required to get those syntaxes works as in the current version.

[vitalybaev]

@kylekatarnls So, exactly when ->prop goes first error is caused in Eloquent. Actually in my case It doesn't matter if ->getProp or ->prop() goes first. Main goal is to have ->prop() before ->prop.

Problem with tests is that we have to include Eloquent implementation to test this case.

I have to use Jade as is in my new project, and don't want use some node.js compilers to blade for that. But I have to have an ability to use all methods from my Eloquent models. That methods are not just getters, it could be more complex with some logic. If there is any way to do so - it would be great!

What about elseif - I could update it, if you wish!

[kylekatarnls]

This bother me because it's a breaking change, users that have objects with both ->prop and ->prop() will break their templates when they will update. Please consider rather the following solutions:

js-phpize:

$pug->setOptions(array(
  'languageExpression' => 'js',
));

or use PHP syntax:

h1 #{object->sayHello()}

[vitalybaev]

@kylekatarnls Thanks for reply.

1) PHP syntax works fine, but templates has warnings in IDE, and arrows not so native way for front-end team.

2) I've set 'languageExpression' => 'js',, but it doesn't work. The same error appears:

ErrorException in HasAttributes.php line 403:
Relationship method must return an object of type Illuminate\Database\Eloquent\Relations\Relation (View: /Users/vitalybaev/Documents/PhpPugBug/resources/views/welcome.pug)

[kylekatarnls]

So it would mean that isset fails, if the Eloquent object would implement __isset and returns false, it would succeed. I will inspect Eloquent code. It could be more relevant to propose them a fix.

[vitalybaev]

Returning false in __isset indeed works, but I think it breaks Eloquent relationships, that are defined as methods, but calls as property

[kylekatarnls]

Yes, this break: https://github.com/laravel/framework/blob/5.4/src/Illuminate/Database/Eloquent/Concerns/HasAttributes.php#L398

Because of: https://github.com/laravel/framework/blob/5.4/src/Illuminate/Database/Eloquent/Model.php#L1300

I think it need one more test before doing $this->$method().

A non-breaking change for your case would be:

use Illuminate\Database\Eloquent\Model;

PugModel extends Model
{
  public function __isset($attribute)
  {
    return !method_exists($this, $attribute) && parent::__isset($attribute);
  }
}

Then you can extend PugModel instead of Model in your code.

[vitalybaev]

Yes, I've already tried this. Thanks!

My model now looks like this (not very pretty):

    /**
     * Get the comments for the blog post.
     */
    public function posts()
    {
        return $this->hasMany('App\Post', 'author');
    }

    public function __isset($attribute)
    {
        if (method_exists($this, $attribute)) {
            if ($attribute == 'posts') {
                return true;
            }
        }
        return !method_exists($this, $attribute) && parent::__isset($attribute);
    }

But this only works fine both in Laravel and Pug

[vitalybaev]

I'm closing this, cause we couldn't fix the problem this way. Thanks for your attention