Compiler does not output "Core" dependencies (jQuery with a 'relative' path)

[christophherr]

https://community.getbeans.io/discussion/compiling-wordpress-scripts-breaking-uikit-elements/

~~This looks like a regression in 1.5 that~~ potentially affects/breaks Beans websites when assets are flushed/recompiled.

I just checked. This didn't work in 1.4 either.

When the jQuery dependency is processed, combine_fragments() calls get_content() which calls get_internal_content(). get_internal_content() checks if the file_exists. Because the path is just /wp-includes/js/jquery/jquery.js, it fails the test and beans_url_to_path() is called, which again returns /wp-includes/js/jquery/jquery.js . And that fails the final file_exists check again. jQuery is not added to the resulting file.

https://github.com/Getbeans/Beans/pull/328 attempts to fix this in beans_url_to_path().

@hellofromtonya Is this a scenario that beans_url_to_path() should handle to begin with or should this be handled in get_deps_to_be_compiled(), e.g by prepending the site_url (or simply '.') to dep_src when necessary?

[christophherr]

Testing locally on Windows: Website running on the native Windows filesystem (Laragon) does not add jQuery Website running on Local by Flywheel adds jQuery

Test website on Ubuntu server (Vultr/Serverpilot) does not add jQuery

[christophherr]

I don't like the idea of beans_url_to_path() handling this because it is a path not an URL. Changes to get_deps_to_be_compiled() won't handle any other Core registered scripts (e.g. jquery-ui-core,...).

Right now, I prefer adding an additional check to get_internal_content() because only affected systems will run through it.

This WIP branch https://github.com/Getbeans/Beans/compare/development...christophherr:check adds a probably too simplistic check/"solution" to get_internal_content().

Another WIP branch https://github.com/Getbeans/Beans/compare/development...christophherr:compiler-path adds an additional check and runs through beans_path_to_url() before passing through beans_url_to_path() again. This approach should be more resilient but also slower.

@iCaspar Could you please give this a sanity check?

[iCaspar]

I agree that the place to check is get_internal_content(). But converting from path to URL and back looks too expensive. If there is a way to avoid that, we should. Is the issue here always with content that's coming from /wp-includes/? If so, maybe the check is:

if ( false !== strpos( $fragment, '/wp-includes/' ) ) {
  $fragment = beans_sanitize_path('.' . $fragment );
}

[christophherr]

Thank you, @iCaspar. I really appreciate your feedback and help.

I agree that path_to_url and url_to_path looks rather clumsy and expensive. The only thing I like about it is that it 'should' cover every edge case...

Scripts can be enqueued with /wp-content/themes/child-theme/app.js but we could say that's 'doing-it-wrong'...

Looking at Core's script-loader.php, /wp-includes/ covers the vast majority of cases, especially for the front-end... /wp-admin/ is used several times as well...

Checking for /wp- might be the way to go...

if ( false !== strpos( $fragment, '/wp-' ) ) {
  $fragment = beans_sanitize_path('.' . $fragment );
}

Updated https://github.com/Getbeans/Beans/compare/development...christophherr:check with this approach.

[iCaspar]

@christophherr This looks like it will probably work 99.44% of the time. The only exception that comes to mind is for those who have changed the default location of their wp-content dir (via define ( 'WP_CONTENT_DIR', 'custom-content-dir' ); or some such) and then try to enqueue with /custom-content-dir/themes/child-theme/app.js.

What if you move the path-to-url round trip idea into its own method: resolve_fragment_location_by_url_parse(). Call it after the check for strpos( $fragment, 'wp-' ) only if that fails, as a last ditch effort -- so we only invest the expensive check if everything else has failed.

[iCaspar]

$fragment = beans_url_to_path( $fragment );
// Fix path on some Windows and Ubuntu systems.
// @ticket 332
if ( ! file_exists( $fragment ) || 0 === @filesize( $fragment ) ) { // phpcs:ignore Generic.PHP.NoSilencedErrors.Discouraged  -- Valid use case.
	if ( false !== strpos( $fragment, '/wp-' ) ) {
		$fragment = beans_sanitize_path('.' . $fragment );
	}
}

if ( ! file_exists( $fragment ) || 0 === @filesize( $fragment ) ) { // phpcs:ignore Generic.PHP.NoSilencedErrors.Discouraged  -- Valid use case.
	$fragment = resolve_fragment_location_by_url_parse( $fragment );
}

// Stop here if it still isn't a valid file.
if ( ! file_exists( $fragment ) || 0 === @filesize( $fragment ) ) { // phpcs:ignore Generic.PHP.NoSilencedErrors.Discouraged  -- Valid use case.
	return false;
}

And

/**
 * Resolve an edge case asset location by running through Beans's URL parsing.
 * 
 * @since 1.6.0
 * @param string $fragment Unresolved asset fragment path.
 *
 * @return string The fragment path as modified by URL parsing.
 */
public function resolve_fragment_location_by_url_parse( $fragment ) {
    $fragment = beans_path_to_url( $fragment );
    return beans_url_to_path( $fragment );
}

[christophherr]

Ha! I was thinking about that this morning. 👍 I am just not sure if we should account for that edge case because I would consider it doing-it-wrong...

I'll wrap this up in a PR including the last ditch effort and Thierry or Tonya can decide. Thanks again, @iCaspar 🥇