contentful / rich-text.php

Utilities for the Contentful Rich Text
https://www.contentful.com
MIT License
12 stars 9 forks source link

Add Twig and Plates extensions #7

Closed dborsatto closed 6 years ago

dborsatto commented 6 years ago

This PR adds support for integrating this library with templating engines Twig and Plates.

codecov[bot] commented 6 years ago

Codecov Report

Merging #7 into master will not change coverage. The diff coverage is 100%.

Impacted file tree graph

@@           Coverage Diff           @@
##             master     #7   +/-   ##
=======================================
  Coverage       100%   100%           
- Complexity      175    181    +6     
=======================================
  Files            60     62    +2     
  Lines           522    540   +18     
=======================================
+ Hits            522    540   +18
Impacted Files Coverage Δ Complexity Δ
src/Bridge/TwigExtension.php 100% <100%> (ø) 3 <3> (?)
src/Bridge/PlatesExtension.php 100% <100%> (ø) 3 <3> (?)

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 5b26b93...46a7f10. Read the comment docs.

dlitvakb commented 6 years ago

I'm not entirely sure that we should provide this integrations within the package, as it means adding external dependencies that may pull in A LOT of framework dependencies that are strictly not necessary for most people that are going to be using this library.

I would go for a separate package for the helpers, or to include it in the current extensions we already have.

dborsatto commented 6 years ago

Actually, they don't add any dependency to users of this library. The dependencies are only included when working on this library, but if you include it in your project, they won't be added (they are in the require-dev section of the config file, not in the require).

It's literally zero cost for the user 🙂

dlitvakb commented 6 years ago

I'm still a bit sceptic about this, as users may have dependency issues if they choose the incorrect bridge, but if you think it's going to be alright, then I'm ok with your decision.

dborsatto commented 6 years ago

Sorry, but there's literally no dependency to this.

The fact that I implement these classes here does not cause any dependency inclusion for the user. Actually, if the user tries to work with one of these bridge classes without having manually installed the required templating engine, they will receive an error because they actually lack the dependency.

dlitvakb commented 6 years ago

That's exactly what I meant though, having the dependency error is not good. Because the unaware user, instead of changing the bridge to the correct one, they may install dependencies to shut the thing up.

dborsatto commented 6 years ago

No, because it will never cause an error if you don't try to use the bridge. This library works completely fine without it, it's built not to force any dependency.

The only error would happen if you intentionally use the TwigExtension and you don't have Twig installed, but if you do so, it's not this library's fault to be honest. It can't cause any sort of conflict because it doesn't define a dependency, and it's also how all packages implement optional dependencies: it's just there, and it's so explicitly opt-in that you must know what you're doing if you use it...

dborsatto commented 6 years ago

This basically letting the user know "Hey, if you already use Twig or Plates in your project, there's this handy extension you can just use", nothing else.