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

Cleanup global $context #2409

Open
nlemoine opened this issue Jan 15, 2021 · 7 comments
Open

Cleanup global $context #2409

nlemoine opened this issue Jan 15, 2021 · 7 comments
Milestone

Comments

@nlemoine
Copy link
Member

Is your feature request related to a problem?

2.x might be a good opportunity to make some spring winter cleaning in the global context provided by Timber.

IMHO, some values are useless:

Some values should be optin/moved to the user scope:

  • body_class: There's many ways to do this. Add it to the global context yourself by using timber/context filter or use {{ fn('get_body_class') }}. One of the reason/benefit of using Timber is that it better decouples PHP from HTML. I hate having to use a WordPress filter (body_class for example) to add a simple CSS class to an element... IMHO, removing this better fits Timber philosophy.

Some values are redundant:

  • Theme instance lives in $context['site']->theme and $context['theme']

Objects should be lazy loaded in case they're not used:

  • theme
  • user
  • site

Some values are missing:

  • query (e.g. $wp_query): very useful because it gives access to conditional tags. Instead of doing {{ fn('is_singular', 'my_custom_post_type') }}, you could do {{ query.is_singular('my_custom_post_type') }} which is more fluent, consistent and nicer to me.
  • debug: less important but sometime useful too

Some may be enhanced:

  • request object is very basic, I don't know if you would be open to adding another dependency but some requests classes would implement stronger features (symfony/http-foundation or laminas/diactoros)

Describe the solution you’d like

A lighter global context with lazy instantiated objects.

In Symfony, the global context is handled by an AppVariable class which accesses services on demand. Although it would be hard to mimic this in Timber, I think the idea behind could be applied.

@nlemoine nlemoine changed the title Cleanup global $context Cleanup global $context Jan 15, 2021
@gchtr
Copy link
Member

gchtr commented Jan 15, 2021

Hell yes, @nlemoine, great ideas! I pretty much agree with everything.

@jarednova might know more about this from the early days of Timber.

  • debug: less important but sometime useful too

What would you put in there?

  • request object is very basic, I don't know if you would be open to adding another dependency but some requests classes would implement stronger features (symfony/http-foundation or laminas/diactoros)

Handling requests probably falls in the same category as Routes. Because WordPress mostly works with rewrite rules and query vars by default, I think we shouldn’t be too opinionated about how to work with requests.

I don’t know if this really needs to be in the context by default. One could argue that you probably shouldn’t work with request variables in Twig, because that’s more PHP land. But we live in an imperfect world and there are surely use cases where this is needed. Also, not everyone cares the same way about separation of concerns as some of us do :). In the end, Timber should make it easier to develop WordPress themes.

I myself would rather have a nice way to work with requests in PHP.

What if we made it optional as well with a list of recommendations (including http-foundation and diactoros) for how to extend Timber?

A lighter global context with lazy instantiated objects.

I agree, more lazyness is nice! But, what we then need to consider is a lot of developers work with dumps to see what’s in a variable. Lazyness hides a lot this information. So we definitely need good documentation about this. And maybe some way to show the information when dumping it (@acobster added some magic for that in https://github.com/timber/timber/blob/2.x/lib/PostArrayObject.php#L41).

I can take care of the documentation part, if you like.

@acobster
Copy link
Collaborator

@nlemoine do you see $context itself becoming an ArrayObject that lazily instantiates stuff as needed?

Planning on digging in a little more next week, but at first glance I think I'm on board with most of this. Like @gchtr, I'm a little on the fence about request.

@nlemoine
Copy link
Member Author

nlemoine commented Jan 15, 2021

Funny story, before opening this issue, I first wanted to submit a PR with some working code that moved $context into its own class extending ArrayObject. 😄

But I faced some issues.

$context = Timber::context(); // Where $context is a class that extends ArrayObject
Timber::render('template.html.twig', $context);

Sadly, you can't pass an ArrayObject to a Twig template. Twig expect a real array.

Using getArrayCopy() could be a workaround but will result in losing all the lazy instantiation benefits, which is one the main goal of this issue.

I'm not sure this can be resolved that way (but I'm very far from being the most skilled citizen in PHP land).

I guess that's why Symfony uses an app key that holds the AppVariable class which allows lazy calls like:

{# Will call getUser() #}
{{ app.user }}

But that would be a breaking change for Timber. You could not call {{ user }} anymore and have to use {{ context.user }} (or whatever name).


By the way, I'm thinking of something, why don't Timber uses addGlobal Twig function? This would avoid the little annoying part of having to get context in every WordPress template files:

$context = Timber::context();
Timber::render('index.html.twig', $context);

Would become:

Timber::render('index.html.twig'); // posts, etc. would already be here \o/

I understand about adding another dependency or being too opinionated. Maybe the Request class could be slightly improved (missing globals like $_COOKIES).

@acobster
Copy link
Collaborator

Sadly, you can't pass an ArrayObject to a Twig template. Twig expect a real array.

Augh, fake polymorphism strikes again. This is my least favorite thing about PHP. :(

Using getArrayCopy() could be a workaround but will result in losing all the lazy instantiation benefits, which is one the main goal of this issue.

Yeah, I don't see a good solution. Since this is fundamentally an issue at the type system level, we'd have to get pretty hacky to get anything like it to work.

By the way, I'm thinking of something, why don't Timber uses addGlobal Twig function?�This would avoid the little annoying part of having to get context in every WordPress template files:

I strongly prefer being explicit. Magic has its own costs that can easily outweigh the annoyance of having an extra line of boilerplate. And since you'd have to loop over the keys to set the globals anyway, I think it would also preclude the laziness you're proposing?

All in all, I think the way we get better laziness given current constraints is by making Theme, User, etc. lazy themselves. This would be more generally useful anyway IMO. Not entirely sure how that would work but it's a pretty large scope and definitely not something I have the bandwidth to implement right now. Just a thought.

@gchtr
Copy link
Member

gchtr commented Jan 17, 2021

I’d like to throw in older discussions about how to handle the context, because it might be worth to revisit how contexts should be handled in the future.

An object-based context has been mentioned quite a few times. I wonder whether that might solve any of the problems we have here? And what other possibilities it would bring?

I don’t want to say that we should tackle a complete context overhaul now, but we might get some idea for future updates that we’d have to consider now.

I strongly prefer being explicit. Magic has its own costs that can easily outweigh the annoyance of having an extra line of boilerplate.

I have to agree with @acobster here.

@gchtr
Copy link
Member

gchtr commented Jan 18, 2021

Planning on digging in a little more next week, but at first glance I think I'm on board with most of this. Like @gchtr, I'm a little on the fence about request.

I thought a little more about keeping request and I think we should keep it for now, because it’s really lightweight and it’s opt-in. With the routes feature, developers only could use the one that Timber provided. They couldn’t switch to another one. With request it’s different, because I can completely ignore it and work with $_GET or $_POST directly or use get_query_var().

I understand about adding another dependency or being too opinionated. Maybe the Request class could be slightly improved (missing globals like $_COOKIES).

If you see areas for improvement, that’s great! I just learned that $_REQUEST also contains $_COOKIES, so that makes sense.

@addedlovely
Copy link

Just came across this whilst doing some performance optimisation. For me the addition of get_body_class - triggers a database call for the 'site_logo' option.

For me personally I never use the body_class function due to all the fluff that WordPress generates there. Would be great if we could get a filter on adding it to the context to just skip that ( even on the V1 branch).
self::$context_cache['body_class'] = implode(' ', get_body_class());

Also really not sure what the point of the wp_title addition is, I've always called {{ function('wp_head') }} like the starter theme - which then pulls in the title. Or is that incorrect?

@Levdbas Levdbas added this to the 2.2.0 milestone May 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants