Content width is not using $GLOBALS['content_width'] when set. about wprig

[mor10]

The jetlag fog has lifted a bit and this makes sense. Unless @bamadesigner has any objections I'm recommending merge.

[mor10]

$cpmtent_width is a royal pain. has been for a long time.

This is how it's currently handled at _s where they've been debating it for years:
https://github.com/Automattic/_s/blob/master/functions.php#L86-L99

On top of that comes Gutenberg which renders this feature more or less obsolete / requires it to change completely. Two relevant tickets:
WordPress/gutenberg#5650
WordPress/gutenberg#6177

I'm inclined to do nothing until Gutenberg sorts itself out because I have a feeling we might get a total rewrite of this entire feature. That said, I'm open to arguments to the contrary.

@hellofromtonya can you refactor your PR to be verbose? The ternary operator is clean, but it's also hard to understand for new/inexperienced developers. If/else statement would be better from a "what is going on here" standpoint.

[hellofromtonya]

I'm inclined to do nothing until Gutenberg sorts itself out because I have a feeling we might get a total rewrite of this entire feature.

Definitely could happen.

That said, I'm open to arguments to the contrary.

Right now, the code doesn't work as it is. Why? The global isn't being used at all. It'll just be a point of confusion for those who build off of this theme.

That means we have a few of choices:

1. Get it to work as it's designed, meaning if it's set, use it; else, use the default.
2. Make it like _s for commonality.
3. Or get rid of the global all together.

Thinking it through, it seems to me there is a reason why for the original design, i.e. to use the global if it was set. Therefore, I'd vote:

[x] we get it working
[x] Then when Gutenberg solidifies, we can revisit if we need to change it.

@mor10 What do you think?

[hellofromtonya]

@hellofromtonya can you refactor your PR to be verbose? The ternary operator is clean, but it's also hard to understand for new/inexperienced developers. If/else statement would be better from a "what is going on here" standpoint.

@mor10 Done. You can view it here in PR #13.

[bamadesigner]

I have it on my list to check out tomorrow. 👍🏻

[bamadesigner]

So, in core, the only place this global is used is in setting the embed width in wp_embed_defaults().

So the more standard approach to solving this problem is simply to filter the 'embed_defaults' filter that's inside that function and set the width inside that filter, e.g.:

function wprig_embed_dimensions( $dimensions ) {
	$dimensions['width'] = 720;
	return $dimensions;
}
add_filter( 'embed_defaults', 'wprig_embed_dimensions', 10 );

Really, then we don't need the "wprig_content_width" filter because people can also use the 'embed_defaults' filter to achieve the same goal. Or we can move that filter here:

function wprig_embed_dimensions( $dimensions ) {
    $dimensions['width'] = apply_filters( 'wprig_content_width', 720 );
    return $dimensions;
}
add_filter( 'embed_defaults', 'wprig_embed_dimensions', 10 );

[mor10]

That makes a lot of sense @bamadesigner. It may also help us work around the inevitable Gutenberg mess which is coming down the track. What say you @hellofromtonya? Can you update your PR to do this new thing?

[hellofromtonya]

Excellent point, @bamadesigner! You are correct that we don't need a separate filter for the width and nor do we need to directly interact with the global either.

I'll update the PR.

[hellofromtonya]

The only enhancement I'd offer is to type hint the incoming $dimensions parameter.