wporg-mu-plugins icon indicating copy to clipboard operation
wporg-mu-plugins copied to clipboard

Remove global header local file dependency

Open Clorith opened this issue 3 years ago • 7 comments

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.

Clorith avatar Feb 18 '22 12:02 Clorith

🤔 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.

iandunn avatar Feb 18 '22 15:02 iandunn

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.

Clorith avatar Feb 18 '22 15:02 Clorith

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

ryelle avatar Feb 18 '22 15:02 ryelle

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

Clorith avatar Feb 18 '22 16:02 Clorith

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 :)

Clorith avatar Feb 18 '22 16:02 Clorith

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 😆).

ryelle avatar Feb 18 '22 20:02 ryelle

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.

dd32 avatar Feb 21 '22 01:02 dd32