pug-php / pug

Pug template engine for PHP
https://www.phug-lang.com
MIT License
391 stars 42 forks source link

Support for magic methods in JS syntax #89

Closed J5lx closed 7 years ago

J5lx commented 7 years ago

(original, misleading description below)

When using JS syntax it is not currently possible to access methods and properties that are handled by magic methods such as __get() or __call(). For example, this code:

= errors.first('email_address')

leads to an error when errors has a __call method but not a first method.


Pretty self-explaining. This code snippet:

if errors.has('email')
  = errors.first('email')

leads to the error Function name must be a string (View: [redacted]/resources/views/signin.pug). Here's the context:

error

When using PHP syntax there's no such error

if $errors->has('email')
  = $errors->first('email')

And for the record, this hybrid syntax works as well:

if errors->has('email')
  = errors->first('email')

So this is probably a bug with the JS-PHP conversion.

kylekatarnls commented 7 years ago

Hi, I'm refactoring js-phpize, the new version should fix that.

kylekatarnls commented 7 years ago

Hi, it should be fixed:

https://github.com/pug-php/pug/pull/93

It should works on both js and php modes.

Please update and try again

J5lx commented 7 years ago

I still get the same error. However, the unit test passes and it looks correct to me, so I'll have a closer look at it later.

J5lx commented 7 years ago

Update: This isn't actually true, see https://github.com/pug-php/pug/issues/89#issuecomment-263287006

FWIW, I just noticed by accident that it's only the expression in the if that fails. This works as well (In latest release at least, haven't tried with issue-90 yet):

if errors->has('email_address')
  = errors.first('email_address')
kylekatarnls commented 7 years ago

Maybe errors is not structured the same way as my test FooBarClass, is it the Laravel standard Error class?

J5lx commented 7 years ago

Thats it! I never even thought of it since stuff like has() seemed so normal and boring, but actually errors is sort of a proxy class that forwards method calls via __call() (See https://github.com/laravel/framework/blob/5.3/src/Illuminate/Support/ViewErrorBag.php#L82). So basically this is all about supporting magic methods for these things. I'll update title and description accordingly. Sorry for not looking closer.

J5lx commented 7 years ago

BTW, https://github.com/pug-php/pug/issues/89#issuecomment-263157508 is wrong since the if in question evaluated to false. When it evaluates to true, the second line causes an error as well.

kylekatarnls commented 7 years ago

The error come from the same reason.

The unit test proove that with classic methods (methods that return true when we use method_exists()), we can use them in if, or in blocks.

But we cannot and we won't support magic methods + JS syntaxes in pug directly. However a pull-request or at least an issue can be open in https://github.com/pug-php/js-phpize for supporting magic methods.

That would mean: replace method_exists in the Dot.h helper (https://github.com/pug-php/js-phpize/blob/master/src/JsPhpize/Compiler/Helpers/Dot.h) with an other mecanism.

But remember, the point is closure and methods are same value type in JS. But in PHP methods are not value type and are different from closure, example:

class Foo
{
  public $one;
  public function __construct()
  {
    $this->one = function () {};
  }
  public function two() {}
  public function __call() {}
}

All can be called the same way:

$foo = new Foo();
$foo->one();
$foo->two();
$foo->three();

Or in JS syntax:

foo.one();
foo.two();
foo.three();

But foo.one is a member, foo.two is a method and foo.three does not really exist. Moreover foo can here be an object or an array. So me must decide of a precedence and test all those possibilityies, also, for the moment, the "call" (the fact to append parentheses after the method/member) does not influence the way we get something in an array or an object when we have a dot.

J5lx commented 7 years ago

I see. I hope I'll be able to get a PR ready soon™, but right now I'm already struggling to find time for my own pet project. Well, we'll see.

kylekatarnls commented 7 years ago

Will be fixed in js-phpize: https://github.com/pug-php/js-phpize/issues/10

kylekatarnls commented 7 years ago

Hi, I enabled get and call in the dot helper of js-phpize, please do composer update to upgrade js-phpize to version 1.1.0 and tell me if you acheive what you wanted with:

if errors.has('email')
  = errors.first('email')
J5lx commented 7 years ago

I still get the same error. For reference, the generated code causing the error looks like this:

<?php if(\Jade\Compiler::getPropertyFromAnything($errors, 'has')('email_address')) { ?><?php echo \Jade\Compiler::getEscapedValue(\Jade\Compiler::getPropertyFromAnything($errors, 'first')('email_address'), '"') ?><?php } ?>

I had a look at getPropertyFromAnything; it doesn't seem to have any logic regarding magic methods and I get the impression that the Pug compiler simply generates it's own getter code without relying on anything from js-phpize.

That's just from a quick first look though, in an hour or two I should have time to look a little closer.

J5lx commented 7 years ago

Okay, I can confirm that magic methods work just fine with plain JsPhpize now. So all that's left should be making them work in pug-php as well. As far as I can see, the compiler generates its own getter code via CommonUtils::getGetter, without any calls to JsPhpize. Then how exactly is this code related to the dot helper in js-phpize? That's the bit I don't quite get right now…

kylekatarnls commented 7 years ago

It's an option. As you say you use JS syntax I thought you already add the option:

$pug = new Pug(array(
  'expressionLanguage' => 'js',
));

This will compile the expressions with js-phpize.

J5lx commented 7 years ago

Oh, that explains it. I wasn't even aware of that option – the laravel package I use ships with a config file, and I blindly assumed it contained every available option, but of course it didn't contain this one. I even came across the code that handled it, but I thought it was some kind of "internal option". Well, I guess it's my fault for making blind assumptions then – sorry for that. After changing the configuration and rewriting the relevant bits to pure JS syntax, everything mostly works except for a few workarounds. I'll submit separate issues (and hopefully PRs) for the remaining issues later.

kylekatarnls commented 7 years ago

It's optional because you can use pug with PHP syntaxes for expressions. The default option "auto" wil just add the minimum of stuf (add $ on variables and some very simples transformations) but it let you enter any valid PHP code. The js will do much more transformations but it means PHP syntaxes are no longer valid (concatenation with +, property getter with ., no declaration of any kind, etc.) but you get closer to pugjs syntaxes. When js-phpize will be perfectly reliable, maybe I will set the option as default. But as it is a breaking change, it will not be for the version 2.

J5lx commented 7 years ago

Thanks for the clarification!