search

pug-php / pug

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

Unexpected override occerd in block scopes #204

Closed narirou closed 5 years ago

narirou commented 6 years ago

Hi, I encountered an issue with the following code:

data

array(
  'text' => '2',
)

pug

mixin paragraph(text)
  p= text
  block

doctype html
html
  body
    +paragraph('1')
      +paragraph(text)
    +paragraph(text)

I expected to get:

<!DOCTYPE html>
<html>
  <body>
    <p>1</p>
    <p>2</p>
    <p>2</p>
  </body>
</html>

But I actually get:

<!DOCTYPE html>
<html>
  <body>
    <p>1</p>
    <p>1</p>
    <p>2</p>
  </body>
</html>

php-pug(3.2.0) will fail, but pugjs will success.

Thanks!

kylekatarnls commented 6 years ago

It works the way you expect in pugjs, so indeed it's a case we didn't anticipated and implemented the wrong way.

I will fix it when I can. I assume there is no emergency as you can work around using an other variable name.

mixin paragraph(_text)
  p= _text
narirou commented 6 years ago

Thanks for quick response! We avoided this problem temporarily by changing the variable name. (However, our app has complex block scopes, so maintenance is a bit difficult.)

dwightjack commented 5 years ago

Hi I can confirm the issue.

Here is a snippet that fails:

mixin Test (params={})
  - const props = params
  p=props.text

mixin Wrap(params={})
  - const props = params
  +Test({ text: "one"})
  if props.label
    +Test({ text: "two"})

+Wrap({ label: "yes"})

Expected output:

<p>one</p><p>two</p>

Actual:

<p>one</p>

In my scenario I heavily use nested mixins and changing variable names would be a bit painful.

Is there any way I could help in fixing this issue?

dwightjack commented 5 years ago

Just to add some more infos. I tested the same snippet useing php as expression language and the result is the same:

mixin Test ($params = [])
  - $props = $params
  p=$props["text"]

mixin Wrap($params = [])
  - $props = $params
  +Test(["text" => "one"])
  if $props["label"]
    +Test(["text" => "two"])

+Wrap(["label" => "yes"])
kylekatarnls commented 5 years ago

This issue is still under the radar but not my top priority, it's a deep change with major impact. I know how to handle it, I just need some time allocation to work on it.

~The work-around in you specific case is variable renaming: mixin Test(testParams = {}), mixin Wrap(wrapParams = {})~

Note than copying the variable to props is not needed and as it's converted into array by PHP, you should avoid it, it uses twice more memory.

dwightjack commented 5 years ago

@kylekatarnls Ok thank you for the prompt reply.

Actually I stripped away a part of the mixin initialization code where I was usinga custom function to provide defaults:

mixin Wrap(params={})
  - const props = assign({ ...}, params)

Where assign maps to array_merge in PHP (my production env) and Object.assign in JS (my development env).

I've experimented a little bit more and looks like that somethign like this would work without changing the variable names:

use \Pug\Pug;

$pug = new Pug([
  'pretty' => true,
  'expressionLanguage' => 'js',
  'cache' => false,
  'globals' => [
    'assign' => function ($src, $arr) {
      return array_merge($src, $arr);
    }
  ]
]);

$html = $pug->render('

mixin Test (params = {})
  - params = assign({}, params)
  p=params.text

mixin Wrap(params = {})
  - params = assign({label: false}, params)
  +Test({ text: "one" })
  if params.label
    +Test({ text: "two" })

+Wrap({ label: true })
');

var_dump($html);

It works correctly with pugjs as well.

Do you see any issue in this implementation?

Thanks again!

kylekatarnls commented 5 years ago

No indeed. I was wrong, the scope problem can happen when using block or creating/changing variables with the same name inside mixins, but not with mixin arguments themsleves.

dwightjack commented 5 years ago

Ok, thank you.

Sorry to bother again, but could you give me an example of this scenario?

the scope problem can happen when using block

kylekatarnls commented 5 years ago

The original message of this issue:

mixin paragraph(text)
  p= text
  block

doctype html
html
  body
    +paragraph('1')
      +paragraph(text)
    +paragraph(text)
dwightjack commented 5 years ago

Got it! Thank you again!

kylekatarnls commented 5 years ago

Hi, it has just been fixed in phug/formatter 0.5.45. Please composer update and retry.

dwightjack commented 5 years ago

Cool! Thank you!

narirou commented 5 years ago

I will test it. Thank you for quick fix!

dwightjack commented 5 years ago

Hi I've tested the example template again but it looks like it is still failing. Rendered HTML is still:

<p>1</p>
<p>1</p>
<p>2</p>

composer.json

{
  "name": "pug-php-test",
  "type": "project",
  "require": {
    "pug-php/pug": "^3.2"
  },
  "authors": []
}

My PHP version is 7.1.19

I've uploaded a test project including vendors here: https://github.com/dwightjack/pug-php-demo

Am I missing something?

kylekatarnls commented 5 years ago

Hi, can you check your versions of the following packages please:

composer show phug/formatter
composer show phug/renderer

I will test your example soon.

dwightjack commented 5 years ago

Hi, thank you for the quick reply!

here is the output:

name     : phug/formatter
descrip. : Pug (ex-Jade) formatter for PHP, HTML template engine structured by indentation
keywords : ast, dialect, html, jade, php, phtml, phug, presentation, pug, render, template, views
versions : * 0.5.45
type     : library
license  : MIT License (MIT) (OSI approved) https://spdx.org/licenses/MIT.html#licenseText
source   : [git] https://github.com/phug-php/formatter.git 59c21f547c4c14a2b13955ee9e2cf5bd0d583244
dist     : [zip] https://api.github.com/repos/phug-php/formatter/zipball/59c21f547c4c14a2b13955ee9e2cf5bd0d583244 59c21f547c4c14a2b13955ee9e2cf5bd0d583244
names    : phug/formatter

autoload
psr-4
* => ./src

requires
php >=5.5.0
phug/dependency-injection ^1.3.0
phug/parser ^0.5.0
phug/util ^0.4.14

requires (dev)
phug/dev-tool ^0.1.0
name     : phug/renderer
descrip. : Pug (ex-Jade) renderer for PHP, HTML template engine structured by indentation
keywords : compiler, dialect, html, jade, php, phtml, phug, presentation, pug, render, template, views
versions : * 0.4.6
type     : library
license  : MIT License (MIT) (OSI approved) https://spdx.org/licenses/MIT.html#licenseText
source   : [git] https://github.com/phug-php/renderer.git 47f4044cdf6d3c2331abec4b8fa5486888a5e2a5
dist     : [zip] https://api.github.com/repos/phug-php/renderer/zipball/47f4044cdf6d3c2331abec4b8fa5486888a5e2a5 47f4044cdf6d3c2331abec4b8fa5486888a5e2a5
names    : phug/renderer

autoload
psr-4
* => ./src

requires
php >=5.5.0
phug/compiler ^0.5.20
phug/util ^0.4.15
symfony/var-dumper ^3.4 || ^4.0

requires (dev)
cebe/markdown ^1.1
js-phpize/js-phpize-phug ^1.1.2
nodejs-php-fallback/coffeescript ^1.0
nodejs-php-fallback/less ^1.0
nodejs-php-fallback/stylus ^1.0
nodejs-php-fallback/uglify ^1.0
phug/dev-tool ^0.1.0
kylekatarnls commented 5 years ago

I cloned your repo, ran index.php then get the bug. Indeed, it works with external variables like this:

$pug->displayFile('template.pug', array(
    'text' => '3',
));

But not with variables created inside the template. I'm not sure we can support this with no risk but I will inspect.

dwightjack commented 5 years ago

Thank you!

Let me not if you need any further test

kylekatarnls commented 5 years ago

Now fixed via propagation level-by-level, so you can take phug/formatter 0.5.46 by running composer update and retry.

dwightjack commented 5 years ago

Thank you! Right now I'm back home. I will try it tomorrow morning.

Thanks again!

dwightjack commented 5 years ago

Hi,

sorry to bother again. I've updated the test code with a more complicated use case and I still get the wrong output. I've updated the master branch of my demo repo (https://github.com/dwightjack/pug-php-demo) but the gist of the update is:

// template.php

mixin Test ($params = [])
  - $props = $params
  p= $props['text']

mixin Wrap($params = [])
  - $props = $params
  +Test(["text" => "one"])
  p= json_encode($props)
  if $props["label"] == true
    +Test(["text" => "two"])

+Wrap(["label" => true])

Expected output:

<p>one</p>
<p>{ "label": true }</p>
<p>two</p>

Actual output:

<p>one</p>
<p>{ "text": "one" }</p>

My next test was to change the name of the $prop variable to something more component specific:

mixin Test ($params = [])
  - $testProps = array_merge([], $params)
  p= $testProps['text']
  block

mixin Wrap($params = [])
  - $wrapProps = array_merge([], $params)
  +Test(["text" => "one"])
  p= json_encode($wrapProps)
  if $wrapProps["label"] == true
    +Test(["text" => "two"])
      +Test(["text" => "three"])

+Wrap(["label" => true])

In this case I receive the correct output:

<p>one</p>
<p>{ "label" : true }</p>
<p>two</p>
<p>three</p>

Not sure if this can help you in further investigating the problem.

Thanks again for your work!

kylekatarnls commented 5 years ago

Note than in this particular case, you don't need to create a new variable as arrays are copied in PHP you can do:

mixin Test ($props = [])
  - $props = array_merge([], $props)
  p= $props['text']

mixin Wrap($props = [])
  - $props = array_merge([], $props)
  +Test(["text" => "one"])
  p= json_encode($props)
  if $props["label"] == true
    +Test(["text" => "two"])

+Wrap(["label" => true])

Still I can try to find why it failed on copying the variable.

dwightjack commented 5 years ago

@kylekatarnls you're right, array_merge is not needed. Thank you for pointing it out.

kylekatarnls commented 5 years ago

Even if you need array_merge (for example to add default values), you can use the same variable name to avoid such conflict.

kylekatarnls commented 5 years ago

In fact, you get similar bug with pugjs:

mixin foo()
  - num = 4
  block
  p= num

mixin bar()
  - num = 5
  +foo()
  em= num

+bar()

It gives <p>4</p><em>4</em>.

So if we fix it, we could create a gap with pugjs. I have a patch. But I must to be sure we want it.

kylekatarnls commented 5 years ago

In fact, it acts like there is no local variable to mixins (except for arguments/attributes) so you're always modifying global variables.

You can test this on pugjs and see you get the same behavior. So we will keep it consistent with pugjs.

kylekatarnls commented 5 years ago

You can use composer require phug/formatter:dev-mixin-local-variables if you really want this behavior, but I will not merge this to a release and I'm not sure to keep this branch.

dwightjack commented 5 years ago

I agree with you. Exposing local variables to the global scope could lead to unexpected behaviors.

One thing you could do is to add to the docs some guidelines about writing pug-php compatible mixins and the rationale about it. JavaScript and PHP handling of parameters is different when it comes to objects and arrays and altering a function parameter is considered an anti-pattern in JS, but with some explanations about how the underlying pug-php engine works the proposed pattern is much more clear.

Thanks again!

kylekatarnls commented 5 years ago

The documentation is open-source and written in markdown, you can make a proposal.