V2 site overwrite magic __call method

[Levdbas]

Related:

• Overwrite __call method
• 2.x Update methods to retrieve site option #1700

Issue

When calling a site option that does not have it's own method, like site.home. Timber would use the magic __call method from the Core class. This magic method by defaults calls the meta() method. Since the meta() method was deprecated in the site class (#1700) the magic __call method throws a deprecation notice.

Solution

Overwrite the __call method in Site.php and call the appropriate option() method instead.

Impact

Less questions on why deprecation notices are thrown.

Usage Changes

None.

Considerations

Sometimes it's hard to wrap your head around what are actual instantiated data in Timber and what is called via these magic methods. I always thought that {{ site.home }} was a valid method, but when digging deeper I found out that you should call {{ site.link }}

I would be in favor of deprecating these magic methods in the future as well so people (including myself) will learn the difference.

Testing

No, should it be tested?

[Levdbas]

@gchtr , Directly created a PR for this. Should we add this to the 2.0 release?

[jrathert]

Sometimes it's hard to wrap your head around what are actual instantiated data in Timber and what is called via these magic methods. [...] I would be in favor of deprecating these magic methods in the future as well so people (including myself) will learn the difference.

I fully support that.

[coveralls]

coverage: 88.528% (+0.003%) from 88.525% when pulling c33ec4d on v2-site-overwrite-call-method into 5149aa7 on 2.x.

[gchtr]

@gchtr , Directly created a PR for this. Should we add this to the 2.0 release?

Thanks for the PR. Yes, because this actually fixes a bug I’d say we should include it in the 2.0 release.

Testing

No, should it be tested?

@Levdbas I think it would be good if we could add a test for this. Do you need help with that?

[Levdbas]

@gchtr , Directly created a PR for this. Should we add this to the 2.0 release?

Thanks for the PR. Yes, because this actually fixes a bug I’d say we should include it in the 2.0 release.

Testing

No, should it be tested?

@Levdbas I think it would be good if we could add a test for this. Do you need help with that?

Alright I created my first test and used the timber compile_string method for that since this call method is only meant to be used directly from Twig. Removing the __call overwrite in the site class causes the test to fail because of the deprecation notice, so I think we are good here. Any feedback on how to improve this test @gchtr ?

[nlemoine]

Thank you @Levdbas! This looks good to me and congrats on writing test 🙂

[gchtr]

This all looks fine, including the test 👍. Thanks, @Levdbas