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

Fix the example 'source' path to work with child themes #678

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from

Conversation

kevinvess
Copy link

By using the get_template_directory() function, it retrieves the absolute path to the directory of the current theme; and if a child theme is being used, the absolute path to the parent theme directory will be returned.

I think it makes more sense to use this method in the example script in case theme developers are just copying this without testing if it works for child themes. I would imagine this gets used for more parent themes than child themes anyway.

@jrfnl
Copy link
Contributor

jrfnl commented Jul 21, 2017

Hi @kevinvess Thanks for your PR. I get where you are coming from, and this is handled by the Custom TGMPA Generator which is the recommended way to download TGMPA.
If you download a copy through the Custom Generator, you will see that the path in the source parameter has been adjusted automatically based on whether you choose parent theme / child theme or plugin.

@jrfnl
Copy link
Contributor

jrfnl commented Aug 3, 2017

@kevinvess Did my response clarify things ? can I close the PR ?

@kevinvess
Copy link
Author

kevinvess commented Aug 3, 2017

Thanks for the response; I do see the Custom Generator does handle applying the correct function depending on how the user intends to use this library.

However, I still think my PR is worth merging. Its a small change that will help ensure that anyone who copies this library from GitHub instead of using your Custom Generator will not run into issues when adding a child-theme later – as is what happened to me.

Do what you will with this, its your code repository.

@GaryJones
Copy link
Member

@jrfnl From your previous stats, do you have any indication about whether TGMPA is included more with parent or child themes?

@jrfnl
Copy link
Contributor

jrfnl commented Aug 3, 2017

@GaryJones I don't currently have those kind of stats, just theme vs plugin and # per version of TGMPA.

There is a sniff proposal is the TRT fork of the WordPress Coding Standards which looks for TGMPA and does some checks. I could add some metrics to that sniff and run it over the theme repo. That should give some indication, though only of the themes published on WP.org - which isn't really all that relevant as themes published on WP.org have to use a version generated using the Custom TGMPA Generator.

Running the sniff over the theme repo will probably take a couple of hours (aside from having to sync the theme repo again first), so it may be a little while before I could have figures.

@GaryJones
Copy link
Member

I consider this to be minor, so if it doesn't get addressed until a later release, so be it.

@jrfnl
Copy link
Contributor

jrfnl commented Aug 5, 2017

Ok, I've ran the metrics for the whole theme repository and posted the results in the sniff thread: WPTT/WPThemeReview#132 (comment)

I have to reiterate that these results are not all that relevant as they are based on the WP.org theme repo where themes are required to use the Custom Generator anyway.

While the results lean towards TGMPA being included more with parent themes than child themes (not surprisingly), the results are not conclusive as the child/parent theme status is unknown of 43% of the themes using TGMPA (= themes using outdated versions and themes using a TGMPA version not generated using the Custom TGMPA Generator, mostly because they were published before it was available).

Thinking this over, these are the potential usage combinations I can think of and whether either function would work out of the box, where the third option is probably least in use:

Theme active Prepackaged plugin provided by Config called from get_template_directory() get_stylesheet_directory()
parent parent parent
child parent parent
child parent child
child child child

If my thinking here is correct (and please verify for yourselves), then based on this table, there is sufficient justification for making this change.

If it would be so decided, however, this PR will need a sister-PR to adjust the logic in the Custom TGMPA Generator before this can be merged.

@GaryJones
Copy link
Member

Thank you for the investigation and write-up.

there is sufficient justification for making this change.

If it would be so decided, however, this PR will need a sister-PR to adjust the logic in the Custom TGMPA Generator before this can be merged.

Let's do it :-)

@jrfnl
Copy link
Contributor

jrfnl commented Aug 6, 2017

OK, so open actions:

  • Rebase this PR so the build can pass
  • Create a PR against the gh-pages branch to update the Custom Generator code (not to be merged until the next version of TGMPA is released)

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

Successfully merging this pull request may close these issues.

3 participants