pattern-lab / patternengine-php-twig

Twig-based PatternEngine for Pattern Lab.
http://patternlab.io/
MIT License
78 stars 36 forks source link

Longhand includes break lineage #36

Open jayfreestone opened 7 years ago

jayfreestone commented 7 years ago

I'm not sure if this is an issue with the twig extension or the core Patternlab library, but using 'longhand' includes with full paths (i.e. not the fuzzy shorthand syntax) seems to break pattern lineage.

Patterns which consist of longhand includes don't show their lineage, and also don't work with the Data Inheritance Plugin, which expects to receive lineage information.

This doesn't seem to be documented as a limitation of using longhand, is this a known issue? Unfortunately using PL in production necessitates the use of full-path includes.

sghoweri commented 7 years ago

@jayfreestone - Unfortunately, this is a well known issue that @EvanLovely and I have discussed quite a bit in the last couple weeks.

While I'm not sure if this will completely fix the Data Inheritance issues, for what it's worth, we think we have a general fix for the terribly broken lineage functionality in the works: Part 1 of our fix, an update to the Twig regex (which is tied in with how Pattern Lab identifies template lineages), is already merged in on our drupal-pattern-lab/patternengine-php-twig fork

Once Part 2's PR (https://github.com/drupal-pattern-lab/patternlab-php-core/pull/5) is merged in downstream, at least basic lineage behavior should be back to normal (w/ embeds and extends also getting picked up by lineages as well now). It might also fix data inheritance.... (or help us get closer to fixing that as well) -- Fingers crossed!

tanc commented 7 years ago

@sghoweri I've just been testing your work, or at least trying to (not very experienced with pattern lab). Using the non-forked repository and aleksip/plugin-data-transform I was able to have data inheritance working correctly but only with the shorthand syntax. As @jayfreestone pointed out in production we need the long syntax. Which led me to here and your forked repo and new commit.

I switched to the forked repo and cloned the commit referenced in your 'Part 2'. I then tried the long syntax but its not picking up inherited data as it should. I've tried both with aleksip/plugin-data-transform and pattern-lab/plugin-data-inheritance but no joy with either. Any guidance?

tanc commented 7 years ago

I realised I had the old regex in my config.yml so I updated it to the new one. Now I'm seeing a lot of errors like:

Warning: array_unique() expects parameter 1 to be array, null given in /Users/tanc/Sites/agile_profile/l.jmp/gitroot/web/themes/custom/jmp_theme/pattern-lab/vendor/drupal-pattern-lab/core/src/PatternLab/PatternData/Helpers/LineageHelper.php on line 220
NULL
PHP Notice:  Undefined offset: 3 in /Users/tanc/Sites/agile_profile/l.jmp/gitroot/web/themes/custom/jmp_theme/pattern-lab/vendor/drupal-pattern-lab/core/src/PatternLab/PatternData/Helpers/LineageHelper.php on line 220
PHP Warning:  array_unique() expects parameter 1 to be array, null given in /Users/tanc/Sites/agile_profile/l.jmp/gitroot/web/themes/custom/jmp_theme/pattern-lab/vendor/drupal-pattern-lab/core/src/PatternLab/PatternData/Helpers/LineageHelper.php on line 220
PHP Notice:  Undefined offset: 3 in /Users/tanc/Sites/agile_profile/l.jmp/gitroot/web/themes/custom/jmp_theme/pattern-lab/vendor/drupal-pattern-lab/core/src/PatternLab/PatternData/Helpers/LineageHelper.php on line 220
PHP Warning:  array_unique() expects parameter 1 to be array, null given in /Users/tanc/Sites/agile_profile/l.jmp/gitroot/web/themes/custom/jmp_theme/pattern-lab/vendor/drupal-pattern-lab/core/src/PatternLab/PatternData/Helpers/LineageHelper.php on line 220

Notice: Undefined offset: 3 in /Users/tanc/Sites/agile_profile/l.jmp/gitroot/web/themes/custom/jmp_theme/pattern-lab/vendor/drupal-pattern-lab/core/src/PatternLab/PatternData/Helpers/LineageHelper.php on line 220
sghoweri commented 7 years ago

Hmmm.

Just to confirm: the goal here is to get PL working, with data inheritance, with lineages working like they would with the shorthand syntax, only using the longhand syntax (hence the whole point of my two PRs), with the data transform plugin also working as expected (also with the include longhand syntax)... that sound about right?

I'm thinking we should break this down into the different moving parts to figure out what (if anything) might need to get updated. For what it's worth, I think I've seen that console message you mentioned when trying to use the longhand syntax with the data transform plugin (but not shorthand) -- we'll need to investigate to see how that plugin + my lineage fix relate.

One other question: do we know if the data transform plugin works as expected using the longhand syntax currently (using the vanilla PL code - not the PR version)?

Sent from my iPhone

On Jun 7, 2017, at 8:54 AM, Tanc notifications@github.com wrote:

@sghoweri I've just been testing your work, or at least trying to (not very experienced with pattern lab). Using the non-forked repository and aleksip/plugin-data-transform I was able to have data inheritance working correctly but only with the shorthand syntax. As @jayfreestone pointed out in production we need the long syntax. Which led me to here and your forked repo and new commit.

I switched to the forked repo and cloned the commit referenced in your 'Part 2'. I then tried the long syntax but its not picking up inherited data as it should. I've tried both with aleksip/plugin-data-transform and pattern-lab/plugin-data-inheritance but no joy with either. Any guidance?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

tanc commented 7 years ago

Chasing my tail a bit here but I think I had removed the regex from config in those errors above. Now with what I think is the correct config:

lineageMatch: |-
  {%([ ]+)?(?:include|extends|embed)(
    |\\()["\\']([\\/.@A-Za-z0-9-_]+)["\\']([\\s\\S+]*?)%}

I get a lot of errors:

Warning: preg_match_all(): Unknown modifier '.' in /Users/tanc/Sites/agile_profile/l.jmp/gitroot/web/themes/custom/jmp_theme/pattern-lab/vendor/drupal-pattern-lab/core/src/PatternLab/PatternData/Helpers/LineageHelper.php on line 216
tanc commented 7 years ago

Confirming the goal is to get data inheritance in pattern lab working as they would with shorthand syntax.

The data transform plugin only works with shorthand syntax (vanilla code).

tanc commented 7 years ago

Is there anything I can do to help move this along?

EvanLovely commented 7 years ago

@tanc We've got this fixed over in our fork of this: https://github.com/drupal-pattern-lab/patternengine-php-twig/pull/1

tanc commented 7 years ago

Great! I'll get to testing it this week

tanc commented 7 years ago

@EvanLovely I've been testing this morning but can't get inheritance to work with your fork. I've created a new issue on your forked repo: https://github.com/drupal-pattern-lab/patternengine-php-twig/issues/4

tanc commented 6 years ago

I've returned to this now that the fork has been merged. I'm still seeing the same problems though:

Warning: preg_match_all(): Unknown modifier '.' in /Users/tanc/Sites/l.responsive_menu/gitroot/web/themes/custom/site_theme/pattern-lab/vendor/pattern-lab/core/src/PatternLab/PatternData/Helpers/LineageHelper.php on line 218

Any clues how to fix?

tanc commented 6 years ago

Ahah, finally I have it working with the lineage helper line in config.yml like this:

lineageMatch: '{%([ ]+)?(?:include|extends|embed)( |\()["\']([\/.@A-Za-z0-9-_]+)["\']([\s\S+]*?)%}'

which after running npm start then gets correctly converted to:

lineageMatch: >-
  {%([ ]+)?(?:include|extends|embed)(
  |\()["\']([\/.@A-Za-z0-9-_]+)["\']([\s\S+]*?)%}

If I pasted in the converted lineage match line it got double escaped and broke.

tanc commented 6 years ago

Have now tested more thoroughly with aleksip/plugin-data-transform and can confirm lineages are working nicely.