Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Special handling of Twig exists in Core #157

Open
NamelessCoder opened this issue Nov 19, 2017 · 9 comments
Open

Special handling of Twig exists in Core #157

NamelessCoder opened this issue Nov 19, 2017 · 9 comments

Comments

@NamelessCoder
Copy link

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:

  • This kind of implementation must not be part of the core repository.
  • It belongs more in the patternengine associated with the template engine
  • This really needs an implementation for such "Helper" style utilities, for example we could be allowed to configure the desired helper class names in the composer configuration we ship with our pattern engines, and ship our own helpers.

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
Copy link
Member

aleksip commented Nov 20, 2017

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
Copy link
Author

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
Copy link
Member

aleksip commented Nov 20, 2017

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
Copy link
Member

aleksip commented Nov 20, 2017

Opened #158.

@aleksip
Copy link
Member

aleksip commented Nov 20, 2017

@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
Copy link
Author

@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
Copy link
Author

@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
Copy link
Member

aleksip commented Dec 14, 2017

@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
Copy link
Author

@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:

  • Code in question:
  • I would add a secondary interface for PatternEngine (for compatibility, to make this opt-in)
  • I would declare a method on said interface that will process like the code above does now
  • I would leave the code in place but give it the possibly transformed pattern name
  • I would refactor the current code into the Twig PatternEngine, implement the new interface and bump version requirements (final step after all the above is released)

Hope that explains the idea!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants