Add support for path namespaces. #1791
Conversation
jenkoian
force-pushed
the
add-twig-namespace-support
branch
from
7e3e172
to
75174ce
Compare
Codecov Report
@@ Coverage Diff @@
## master #1791 +/- ##
============================================
+ Coverage 94.83% 94.84% +<.01%
- Complexity 1527 1530 +3
============================================
Files 48 48
Lines 3585 3591 +6
============================================
+ Hits 3400 3406 +6
Misses 185 185
Continue to review full report at Codecov.
|
|
@jenkoian thanks for the contrib! I'll confess we're deep into work on 2.x so I won't be able to start a review right away. My biggest concern is just double-tripple checking that the use of a namespace will not disrupt rendering a |
Codecov Report
@@ Coverage Diff @@
## master #1791 +/- ##
============================================
+ Coverage 94.83% 94.84% +<.01%
- Complexity 1527 1530 +3
============================================
Files 48 48
Lines 3585 3591 +6
============================================
+ Hits 3400 3406 +6
Misses 185 185
Continue to review full report at Codecov.
|
4 similar comments
Codecov Report
@@ Coverage Diff @@
## master #1791 +/- ##
============================================
+ Coverage 94.83% 94.84% +<.01%
- Complexity 1527 1530 +3
============================================
Files 48 48
Lines 3585 3591 +6
============================================
+ Hits 3400 3406 +6
Misses 185 185
Continue to review full report at Codecov.
|
Codecov Report
@@ Coverage Diff @@
## master #1791 +/- ##
============================================
+ Coverage 94.83% 94.84% +<.01%
- Complexity 1527 1530 +3
============================================
Files 48 48
Lines 3585 3591 +6
============================================
+ Hits 3400 3406 +6
Misses 185 185
Continue to review full report at Codecov.
|
Codecov Report
@@ Coverage Diff @@
## master #1791 +/- ##
============================================
+ Coverage 94.83% 94.84% +<.01%
- Complexity 1527 1530 +3
============================================
Files 48 48
Lines 3585 3591 +6
============================================
+ Hits 3400 3406 +6
Misses 185 185
Continue to review full report at Codecov.
|
Codecov Report
@@ Coverage Diff @@
## master #1791 +/- ##
============================================
+ Coverage 94.83% 94.84% +<.01%
- Complexity 1527 1530 +3
============================================
Files 48 48
Lines 3585 3591 +6
============================================
+ Hits 3400 3406 +6
Misses 185 185
Continue to review full report at Codecov.
|
Codecov Report
@@ Coverage Diff @@
## master #1791 +/- ##
============================================
+ Coverage 94.54% 94.57% +0.02%
- Complexity 1530 1540 +10
============================================
Files 48 48
Lines 3594 3613 +19
============================================
+ Hits 3398 3417 +19
Misses 196 196
Continue to review full report at Codecov.
|
|
@jarednova thanks for taking a look at the PR! Yeah I figured it was all hands to the pump re. 2.x release (what a great 2.x feature namespace support would be btw! 😃). I'll add some more tests later today. |
|
@jarednova I've added a few more tests, let me know if that's what you had in mind 👍 |
Particularly useful if using a styleguide which uses a particular namespace.
jenkoian
force-pushed
the
add-twig-namespace-support
branch
from
3cc500c
to
726a2d2
Compare
|
Yep! Those tests show namespaced and non-namespaced template calls living side-by-side in harmony. I think we're good here, but I'd still like @gchtr or @pascalknecht to give it a look as well to make sure there's something we're not missing/considering |
|
Looks like build failed due to a composer issue on one of the jobs, are you able to restart the job? |
|
@jarednova @jenkoian Codewise it looks good to me. Only objection to merge for me is documentation which is currently missing and code style which is currently not according to WordPress Guidelines. |
|
@pascalknecht agreed! @jenkoian can you format according to WordPress Coding Standards and update the docs? I think this would belong in the https://github.com/timber/timber/blob/master/docs/guides/template-locations.md |
|
Oh, this is a really nice addition! I just have one question: In the linked Symfony documentation, the pattern to describe namespaces is defined through |
|
I'd echo @gchtr about following the Symfony convention. Anywhere Timber can follow existing conventions would be fantastic! |
|
Thanks for your comments!
Docs now added, have amended code style, although it doesn't look like this is enforced throughout the lib?
Excellent spot! 🦅 👁 I initially planned to follow what symfony do (i.e. path => namespace) but I didn't think it made as much sense here as we're not configuring via yaml. For example, in Symfony you can imagine.. 'path/to/templates1': namespace1
'path/to/templates2': namespace2
'path/to/templates3': ~Would map to PHP as.. array(
'path/to/templates1' => 'namespace1',
'path/to/templates2' => 'namespace2',
'path/to/templates3' => null,
);Which makes looping over etc. really easy. However, if you imagine the following... Timber::$locations = array(
'path/to/templates1' => 'namespace1',
'path/to/templates2' => 'namespace2',
'path/to/templates3',
);We'd end up with... array(
'path/to/templates1' => 'namespace1',
'path/to/templates2' => 'namespace2',
0 => 'path/to/templates3',
);So we end up with the path being the key in some instances and the value in others, we can of course code around that, but it seemed simpler to me to just flip them around. However I noticed Symfony supports multiple paths per namespace, which obviously won't work if we have the namespace as our key as we can't have duplicate keys. So, I've gone ahead and made this change. It's quite involved unfortunately, but all the tests pass. Only concern I have is that it will potentially change the following filters:
As previously they would get a list of paths like so: array(
'path/of/templates1',
'path/of/templates2'
);Where as now it'll be: array(
'__MAIN__' => array(
'path/of/templates1'
),
'namespace' => array(
'path/of/templates2'
)
);Finally, I've added a test for multiple paths per namespace and I've also thought of nested namespaces, so I've added a test to cover that too. Cheers! |
| }, $path_locations ); | ||
| } else { | ||
| $fs->addPath( $path_locations, $namespace ); | ||
| } |
There was a problem hiding this comment.
I'm being overly defensive here as $path_locations will always be an array, as it affects code coverage I'll remove this if/else.
There was a problem hiding this comment.
Have removed the else, think the if is fine.
lib/Timber.php
Outdated
| include trailingslashit( $uri_locations ) . $sidebar; | ||
| $found = true; | ||
| break; | ||
| } |
There was a problem hiding this comment.
I'm being overly defensive here as $uri_locations will always be an array, as it affects code coverage I'll remove this if/else.
|
Ok so it will affect the filters you mentioned: We will need to include this in the upgrade guide to 2.0 then. Agreed @jarednova ? |
lib/LocationManager.php
Outdated
| continue; | ||
| } | ||
| $theme_locs[] = $root; | ||
| $root = trailingslashit($root); | ||
| $theme_locs[ FilesystemLoader::MAIN_NAMESPACE ][] = $root; |
There was a problem hiding this comment.
This might be undesirable if a user is using a custom loader (via a filter) and have customised the main namespace name. Should a define a constant on the Loader class within Timber rather than use the one from the Twig Filesystem Loader?
As custom loaders may be used, limit the dependency on FilesystemLoader in the LocationManager by defining our own main namespace.
|
Cool stuff 😎. This looks good for me in terms of documentation. I have to admit that the Loader isn’t my strongest suit, so I’d be happy if someone else could look at the code in detail, @jarednova maybe? If the filters are going to change, we definitely need to add this in the Upgrade Guide for 2.0. I normally do this separately, as soon as a feature is merged. Currently, this pull request wants to merge into |
|
Do you want me to rebase against 2.x ? |
lib/LocationManager.php
Outdated
| $locs = array_merge($locs, self::get_locations_theme()); | ||
| $locs = array_merge($locs, self::get_locations_caller($caller)); | ||
| $locs = array_unique($locs); | ||
| //$locs = array_diff_assoc( $locs, self::get_locations_theme() ); |
There was a problem hiding this comment.
Just to explain, I don't think the diff is needed, it's handled in the array merge and array unique.
array_diff() not needed as is handled by a combo of the array merge and array unique.
|
@jenkoian thanks so much for hanging with us through all these Qs. I think we're very close, but I get caught on the issue of the filters and how it could potentially break compatibility with anyone who's using I just wrote a test in 9f4ed02 to validate the current behavior and see how it compares to this PR (on my local the new test fails against this PR). I think there are two routes to go to merge this in:
My vote is for #2; but I want to see what @jenkoian thinks of this |
|
Also voting for option 2 |
|
Awesome test! I'll take a closer look and get back to you 👍 |
|
Ok had chance to review the failing test. Yes agree with your two options, I am happy to add the backwards compatibility workaround if you'd like, but yeah would probably vote for option 2. I guess one thing on that, is it would require the user to add a namespace, even if using the default, e.g.
as opposed to simply
They would also need to ensure they're appending to an existing array (or passing an array if using a new namespace they've created). Not sure if we deem this too much of a departure even for 2.x or not? I'm not sure how used these filters are for the community? I guess as long as we document the filter and it's usage it should be ok, I can't seem to find much docs on the filter at the mo, so perhaps that's something else I can add? |
|
I also vote for option 2. In addition to that, could we get rid of the following pattern, where locations are assigned through a static variable? Timber::$locations = array(
'path/to/templates1' => 'namespace1',
'path/to/templates2' => 'namespace2',
'path/to/templates3',
);Instead, I’d only use the
This is also a reason why I think using a filter only would be better, because that’s the established behavior for filters: You filter something, or you append to something existing. If we provide good examples, I don’t see a problem. For the deprecation, I’d do it like this:
@jenkoian There’s not so much documentation about the filters yet, but if you’re going to rebase against 2.x, then you’re gonna see that there’s the base for the upcoming filter documentation (read more about this here. timber/lib/LocationManager.php Lines 25 to 34 in 7efcfb2 I’d be very happy if you could complete that with descriptions and examples, etc. We’d still have the main documentation in the Template Locations Guide. The filter documentation is more some kind of technical reference. |
|
@gchtr that is an awesome approach (love this project!). When I get some time i'll add the filter documentation. I've not rebased this against 2.x yet, do you want me to do that now, or should I open up a new PR with this against 2.x for comparison? |
|
@jenkoian please open a fresh PR against 2.x (referencing this PR), I think things will be easiest to track there — thanks! |
|
Ok opened against 2.x here: #1811 struggling for time a bit at the mo, so not had chance to make the recommended changes. Will do so as soon as I can! |
Issue
When looking into timber for a project I'm currently working on, I was looking to make use of path namespaces (e.g. https://symfony.com/doc/current/templating/namespaced_paths.html#registering-your-own-namespaces) but it seems that Timber doesn't currently support this.
Solution
This patch introduces support for path namespace, at time of writing only as part of user defined locations, however this could easily be added to to support namespace in theme dirs as well if you wanted.
Impact
It should be backwards compatible, so it shouldn't have any negative impact on existing users.
Update This will have an affect on users of some filters, see #1791 (comment)
Usage Changes
Users who wish to make use of path namespaces can now do so, e.g. with a styleguide namespace
Considerations
I've not spent too much time on this patch, I'm hoping for some guidance/advice if there are any edge cases I may not have considered as I am very new to this library.
It's also worth noting that I've just checked for a namespace presence and ignored if none there, but we could easily change to give each location the default namespace (__MAIN__) if none is configured, to cut down on an if statement.Update This now does indeed set the default namespace.
Testing
I included one basic unit test, but would be happy to add more if you think this patch is worth including.Update Have included several useful tests.