WordPress / wporg-mu-plugins

Development of the Global Header and other mu-plugins used on WordPress.org.
64 stars 29 forks source link

Remove global header local file dependency #169

Open Clorith opened 2 years ago

Clorith commented 2 years ago

The latest change to the global headers in 917ed580530f02be0849c5609cb3312375a0fe12 introduced a dependency warning for a local WPORGPATH, making the header cause errors in local environments for single components of the wporg network that may be pulling them in.

In a perfect world, we'd have a file_exists check here, and if not, fall back to a fixed digit, as anyone working in a local environment would (hopefully) be familiar with the concept of flushing their caches when needed.

iandunn commented 2 years ago

🤔 It seems reasonable to me to assume that local envs have the same constants available that production does. I feel like adding safety checks for every instance of this situation would lead to too much clutter, and it'd hide the fact that there's something missing from the env. Triggering an error lets the dev know that there's something they should fix.

I'm not sure this function should be loading the CDN version of the file in local envs, since people will need to edit it every now and then. It seems like it'd be better to load the local version in that local/staging, and the CDN in production. cc @dd32 who's working on some changes to how CDN versions are enqueued.

Clorith commented 2 years ago

In my case, I have limited sets of files, and grabbing the header for an external environment for the support forums, which throws a warning since the WPORGPATH doesn't exist outside of the network, and if I set this to be any value, then the global styles won't be fetched, since they depend on a check for that same constant to know to fetch them remotely.

ryelle commented 2 years ago

Would https://github.com/WordPress/wporg-mu-plugins/pull/149 help the issue at all? (for the styles loading)

Clorith commented 2 years ago

Yes, since we'd use local otherwise, that would be spot on 👍

Clorith commented 2 years ago

Actually, that would be better overall, but wouldn't solve this specific issue, since the filemtime check would still be there for the wp4-styles.

It feels a bit overkill to pull in trunk/wordpress.org/public_html/style, but we're already doing that for the wporg theme and mu-plugins/pub, so I guess it could be added as a dependency for a standalone env as well, I had just hoped not to need it :)

ryelle commented 2 years ago

I think a file_exists sounds like a good idea — especially since the enqueue loads the file from the CDN, so the local version wouldn't necessarily correspond to the live version (unless you're better than I am at keeping dependencies updated 😆).

dd32 commented 2 years ago

We need to include some kind of cache-busting version here.

I would be tempted to set it to something like: $version = defined(...) && file_exists(...) ? filemtime(...) : ( time() % HOUR_IN_SECONDS );

That accounts for the fact that the CDNised script will be updated occasionally (and bumping the version here is just too much effort), but only requires the client to pull a new stylesheet every hour, while production will always output the exact version.