Wp-rocket: Combine Google Fonts causing 404 error in specific conditions

[piotrbak]

Porto theme is adding Google Fonts using unexpected (incorrect) markup:
//fonts.googleapis.com/css?display=swap&family=Open+Sans:200,300,400,700,800,600,|Shadows+Into+Light:200,300,400,700,800,600,|Oswald:200,300,400,700,800,600,|&subset=cyrillic,cyrillic-ext,greek,greek-ext,khmer,latin,latin-ext,vietnamese

Note the | after the definition of the last, Oswald font.

Correct markup:

When our combination feature is enabled, we're not removing this | but appending another | with font definition. It leads to the 404 error, this is how it looks like after combination:

Their support team claims that it should be fixed on our end, advising the customer to deactivate our plugin.

Related ticket:
https://secure.helpscout.net/conversation/1082860619/146039?folderId=2415573

Backlog Grooming

• [x] Reproduce the problem
• [x] Identify the root cause
• [x] Scope a solution
• [x] Estimate the effort

[piotrbak]

Steps to reproduce:

• Enable Porto Theme
• Embed Google font using default code
  <link href="https://fonts.googleapis.com/css?family=Montserrat&display=swap" rel="stylesheet">
• Make sure that it's added after Porto's fonts
• Enable Combine Google Fonts feature

Our main Mega site is ready for testing, please uncomment the 11th line of header.php file of Porto theme.

[crystinutzaa]

Reproduce the issue ✅
Reproduced on mega.wp-rocket.me

Identify the root cause ✅
This is the line:
https://github.com/wp-media/wp-rocket/blob/a37e46f58259e38dbf895061536551763f8aace7/inc/classes/optimization/CSS/class-combine-google-fonts.php#L131-L132

In Porto theme, the last font adds a |
When we combine Porto theme fonts with other google fonts, this will break the format because will contain 2 ||.

Scope a solution ✅
We need to parse the list of $this->fonts and remove from the end any | or %7C, so the double | will never appear while we implode this array.

Estimate the effort ✅

Effort: [S]

The code change itself is very low effort.
Integration tests should take longer, because those which are available are not ok.

@hellofromtonya I would need directions for Integration tests

[hellofromtonya]

@crystinutzaa I can help guide you on the integration tests. The time effort for the integration tests is [S].

Please update the effort estimate using our t-shirt sizing story point system.

[hellofromtonya]

@crystinutzaa For the integration tests, I'd suggest the following:

• Use the Google Fonts <link href> that @piotrbak identified above, i.e. that you validated when replicating the issue. That's your test HTML data.
• Add the new test.

Looking at the existing 2 tests, we're redundant. What do you think about adding a data provider instead to remove the file_get_contents() repeating code within the tests?

[arunbasillal]

@hellofromtonya

For the proposed solution:

We need to parse the list of $this->fonts and remove from the end any | or %7C, so the double | will never appear while we implode this array.

Wouldn't it be faster to check the result after implode and replace any double || or %7C%7C with a single?

And on second thought:

• Shouldn't this be something that should be fixed at Porto's end?
• We can try and reach out to them and talk about this.

[hellofromtonya]

Shouldn't this be something that should be fixed at Porto's end?

@arunbasillal Yes, I think so. In general, Rocket shouldn't be the fixer for everything external to it.

In reviewing this edge case, we actually improved the source code and made Rocket more robust. As a result, it also protected against this particular issue.

But yes, Porto should also fix this for their customers/users.