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

When validating .min assets, check plugins_url(). #518

Merged

Conversation

boonebgorges
Copy link
Contributor

Previously: #495

Plugins like the-events-calendar load JS assets like this:

  • Tribe__Assets::register() calls tribe_resource_url() generate an expected URL for the asset
  • tribe_resource_url() builds an asset URL using plugins_url()
  • This generated URL is then validated in Tribe__Assets::maybe_get_min_file(), which uses (among other things) WP_PLUGIN_URL

When WP_PLUGIN_URL and plugins_url() diverge, Tribe cannot find the assets, and they're not loaded. In my case, the divergence is because of the wordpress-mu-domain-mapping plugin (plugins_url() returns the mapped domain, while WP_PLUGIN_URL uses the primary URL of the installation), but it can happen whenever plugins_url is filtered.

This PR contains an ugly but simple workaround: Yet Another String Comparison, this one using plugins_url(). It's homely, but it works.

More generally, I wonder if maybe_get_min_file() is too aggressive in its validation. By simply returning false when it can't validate a file, it creates situations that are really hard to debug. It seems like it might be easier if the URLs were simply allowed to pass through, so that WP would enqueue the assets, which would then 404.

Alternatively, it may be useful to refactor maybe_get_min_file() so that, instead of simply bailing, any return value - false or a URL - is passed through a filter. This way, unorthodox setups could override the otherwise binding decisions of tribe-common :)

Thanks for considering!

@sc0ttkclark
Copy link
Contributor

👍

@borkweb borkweb added code review Status: requires a code review. needs release Needs an associated release in Central before merging. needs ticket Needs an associated Central ticket before merging. labels Sep 19, 2017
@borkweb borkweb requested a review from elimn September 19, 2017 11:22
@bordoni bordoni changed the base branch from master to release/M17.19 September 20, 2017 00:48
@barryhughes
Copy link
Contributor

🎫 #89228

@barryhughes barryhughes removed the needs ticket Needs an associated Central ticket before merging. label Sep 22, 2017
Copy link
Contributor

@elimn elimn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for pursuing this @boonebgorges, we really appreciate the help in addressing some less common configurations.

Not long after your last PR, we discussed this method. In the long term we would like to replace it. The current one tries to reverse a URL to a filepath, but due to aliasing, filters, etc. this is all ultimately guesswork. Instead of having a method which accepts a URL and tries to reverse it, we want a method which accepts a path and outputs a URL. Then we can skip all of this convoluted business.

In the mean time, improving this method is most welcome. We would love to include your contribution here. Thank you very much!

@elimn elimn added merge Status: ready to merge. and removed code review Status: requires a code review. needs release Needs an associated release in Central before merging. labels Sep 26, 2017
@elimn elimn merged commit 7b4d85f into the-events-calendar:release/M17.19 Sep 26, 2017
@boonebgorges
Copy link
Contributor Author

Thanks very much, @elimn! Feel free to ping me if you'd like feedback on potential solutions from someone who maintains non-standard setups.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merge Status: ready to merge.
Projects
None yet
5 participants