pattern-lab / patternlab-php-core

This repository provides the core functionality for Pattern Lab. It is meant to be used from within an Edition with a PatternEngine and StarterKit.
http://patternlab.io/
MIT License
43 stars 62 forks source link

Special handling of Twig exists in Core #157

Open NamelessCoder opened 6 years ago

NamelessCoder commented 6 years ago

Hi Pattern-Lab folks,

I've created a Pattern Lab Fluid Edition - Fluid is the templating language of the TYPO3 and Neos CMS'es - and in that regard I came across a rather unfriendly implementation in your LineageHelper class.

It appears this class has special handling for parsing Twig lineages, which is a bit sad when you're coming to Pattern-Lab with an implementation for an additional template language and then find that you can't achieve the same level of integration because special respect was given to Twig.

In my humble opinion:

In the case of lineages in particular, this becomes important, as different template engines "include" or "render" or "extend" files using different syntaxes - which you currently are unable to support because of this hidden pseudo-dependency between your core and twig pattern engine implementations.

aleksip commented 6 years ago

Hi! First of all, very exciting to learn about the Pattern Lab Fluid Edition you have created. The Pattern Lab ecosystem might seem a bit confusing at first, but it does make exactly this kind of extensibility possible.

I agree that Pattern Lab core should not have any code specific to a particular templating engine in it, and that core should not favour any templating engine over another. Does the Twig specific code in LineageHelper cause problems for your Fluid implementation?

Pattern Lab's architecture includes plugins and an event-based way to extend core. I think that especially this part of Pattern Lab should be better documented, but you can look at existing plugins for examples.

For lineage specific stuff a plugin can listen for the patternData.lineageHelperStart and patternData.lineageHelperEnd events. PatternEngines can have listeners too, so you could have a PatternLabListener.php in your patternlab-fluid-patternengine.

NamelessCoder commented 6 years ago

Hi @aleksip,

Does the Twig specific code in LineageHelper cause problems for your Fluid implementation?

In this case, the specific problem is that PL matches a certain character, @, if it is first byte of the pattern name, and then processes the include/render statement in a special way to end up with the proper patterntype-patternname style identification of the pattern.

Fluid has no such marker and is not able to process render statements for path starting with a @ - but as far as I can tell, if PL was able to do this processing on a Fluid f:render partial="$name" syntax, the processing would result in the proper pattern name.

I'm not sure if the events will let me do what is necessary, but I'll certainly take a look. Thanks for the hint!

If it does serve my purpose then I'd suggest moving the Twig-specific chunk of processing into a similar event listener in the Twig pattern engine.

aleksip commented 6 years ago

I'm not sure if the events will let me do what is necessary, but I'll certainly take a look. Thanks for the hint!

Do let us know if it works for you! I think it is great to have a PatternEngine for Fluid, and that it is important for the whole project to get it working.

If it does serve my purpose then I'd suggest moving the Twig-specific chunk of processing into a similar event listener in the Twig pattern engine.

Indeed, we should do that. Will open an issue about this.

aleksip commented 6 years ago

Opened #158.

aleksip commented 6 years ago

@NamelessCoder Well, I guess I was a bit too fast to open another issue, as this issue is more or less about the same thing, but maybe we can use this issue to track your progress with specific requirements related to the Fluid edition? :)

NamelessCoder commented 6 years ago

@aleksip I'll keep you updated here in this issue - but there may be a couple of days between the updates from me, I'm working on this project in smaller steps when there's time left over.

NamelessCoder commented 6 years ago

@aleksip I think I've gotten around most of the pattern resolving that was made harder by the special processing here. The implementation I have is still far from perfect though.

I've considered perhaps adding a public method to the PatternEngine component to offload this "resolve pattern name" logic, since it doesn't really fit to make an event listener responsible for this (IMHO, it should be an integral part of the PatternEngine's capabilities).

Would this be a fair way to solve that? If you agree, I would make a pull request for that (and one to follow up with refactoring the Twig handling to the Twig PatternEngine).

aleksip commented 6 years ago

@NamelessCoder Thanks for the update! I must admit that I still haven't completely grasped the problem you are having, and the way you are proposing it should be fixed... Could you please flesh out your idea a bit more or (if it isn't a huge amount of work) submit a PR for further discussion? Would it be possible to implement your solution in a backwards compatible way?

NamelessCoder commented 6 years ago

@aleksip I would definitely prefer to base the discussion on a pull request, just wanted to make sure I don't waste effort. It would indeed be possible to implement it backwards compatible, and keep the (then deprecated) handling in place for as long as you need.

Rough sketch:

Hope that explains the idea!