Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

RUCSS wrong home_url detection with TranslatePress enabled #5640

Closed
4 tasks
NataliaDrause opened this issue Dec 22, 2022 · 11 comments · Fixed by #5896 · May be fixed by #6266
Closed
4 tasks

RUCSS wrong home_url detection with TranslatePress enabled #5640

NataliaDrause opened this issue Dec 22, 2022 · 11 comments · Fixed by #5896 · May be fixed by #6266
Assignees
Labels
3rd party compatibility Issues related to 3rd party compatibility like theme, plugin or hosting effort: [XS] < 1 day of estimated development time module: remove unused css priority: low Issues that can wait type: enhancement Improvements that slightly enhance existing functionality and are fast to implement

Comments

@NataliaDrause
Copy link
Contributor

Before submitting an issue please check that you’ve completed the following steps:

  • Made sure you’re on the latest version - 3.12.3.3
  • Used the search feature to ensure that the bug hasn’t been reported before - yes

Describe the bug
RUCSS is failing for all language homepages when TranslatePress plugin is used.
This is the same issue as shared here: #5065

The same solution provided by Ahmed resolves the issue:
#5065 (comment)

To Reproduce

  1. Install and activate TranslatePress plugin
  2. Add a language with a slug and save settings:
    image
  3. Enable Remove Unused CSS and visit URL like https://example.com/fr/ in the Incognito browser.
  4. Wait for RUCSS to process this page.
  5. The generation will fail at all attempts with 400 Bad json error.

Expected behavior
Used CSS should be generated with no issues for language homepages.

Additional context
Ticket: https://secure.helpscout.net/conversation/2102001423/390024/
Slack: https://wp-media.slack.com/archives/C029G1B5HD2/p1671727809871749

Backlog Grooming (for WP Media dev team use only)

  • Reproduce the problem
  • Identify the root cause
  • Scope a solution
  • Estimate the effort
@piotrbak piotrbak added 3rd party compatibility Issues related to 3rd party compatibility like theme, plugin or hosting type: enhancement Improvements that slightly enhance existing functionality and are fast to implement priority: low Issues that can wait module: remove unused css labels Dec 26, 2022
@piotrbak piotrbak added this to the 3.13.4 milestone Apr 14, 2023
@piotrbak piotrbak removed this from the 3.13.4 milestone Apr 23, 2023
@piotrbak
Copy link
Contributor

Acceptance Criteria:

  1. Used CSS should be generated for the different language homepages that are stored in the subdirectories with TranslatePress
  2. Used CSS should be generated correctly for translated pages different than homepage with TranslatePress
  3. Used CSS should be generated correctly for regular, not translated homepage and other pages with TranslatePress
  4. No regressions in generating Used CSS without TranslatePress

@CrochetFeve0251 CrochetFeve0251 added GROOMING IN PROGRESS Use this label when the issue is currently being groomed. needs: grooming and removed needs: grooming GROOMING IN PROGRESS Use this label when the issue is currently being groomed. labels Apr 26, 2023
@CrochetFeve0251
Copy link
Contributor

Scope a solution

To fix that issue we can create a thridparty class that detect TranslatePress by checking if the constant TRP_PLUGIN_VERSION is defined.
Then we will create an callback on the rocket_rucss_is_home_url with the following logic:

	add_filter('rocket_rucss_is_home_url', function( $is_home, $url ) {
	
	$url_converter =  new TRP_Url_Converter( new TRP_Settings() );
	$language = $url_converter->get_lang_from_url_string($url);
	
	$url_language = $url_converter->get_url_for_language($language);

	return $url === $url_language ? $url_language : $is_home;
	}, 10, 2);

Estimate the effort

Effort S

@vmanthos
Copy link
Contributor

@piotrbak I am re-opening this one as the issue is still happening with 3.14.3.

According to @mostafa-hisham, we send the request to the queue which is for is_home but when we fetch the data we check the non-home queue.

After adding error_log() statements here:

On the first pass $url, $url_language, and $home_url have the correct value for the additional language, e.g. https://example.com/fr.

When retrying, only the $url is correct. The other two values are that of the default language: https://example.com/.

I rolled back TranslatePress to 2.5.4 in case they changed something on their side but the issue is still there.

@vmanthos vmanthos reopened this Aug 10, 2023
@piotrbak piotrbak removed this from the 3.14 milestone Aug 10, 2023
@piotrbak piotrbak added needs: grooming and removed effort: [S] 1-2 days of estimated development time labels Aug 10, 2023
@remyperona
Copy link
Contributor

Reproduce the issue ✅

Identify the root cause ✅

I think this is coming from the context, front or admin. When on admin the home URL is not rewritten, so we don't get the expected values.

Scope a solution ✅

There is a filter we can use, and that is already in use in other methods of the compatibility class trp_add_language_to_home_url_check_for_admin.

Using add_filter( 'trp_add_language_to_home_url_check_for_admin', '__return_false' ); before using the methods from the converter class should make sure we get the expected values.

We can do remove_filter( 'trp_add_language_to_home_url_check_for_admin', '__return_false' ); after.

Estimate the effort ✅

Effort [XS]

@remyperona remyperona added the effort: [XS] < 1 day of estimated development time label Aug 14, 2023
@engahmeds3ed
Copy link
Contributor

Good catch @Tabrisrp

Also I think we can use the method get_home_url_for_lang instead of the line:

$url_language = $converter->get_url_for_language( $language, home_url() );

[Enhancement] and I can see a room for enhancement by making private properties for mostly used variables like

$translatepress = TRP_Translate_Press::get_trp_instance();

and

$converter      = $translatepress->get_component( 'url_converter' );

this is used everywhere, what do u think?

@CrochetFeve0251
Copy link
Contributor

CrochetFeve0251 commented Oct 19, 2023

Scope a solution

The first step will be to change the table to add a new is_home field.

For that we will have to change the inc/Engine/Optimization/RUCSS/Database/Tables/UsedCSS.php class to change the schema and add the migration logic.
Then we will have to change the schema to add the new field inc/Engine/Optimization/RUCSS/Database/Schemas/UsedCSS.php.
We will have to do the same for the rows in inc/Engine/Optimization/RUCSS/Database/Row/UsedCSS.php.
Finally we will have to add the is_home parameter to the create_new_job method.

The second step is to reuse that new logic inside the controller.
For that we need to add the is_home when creating the row and in check_job_status reuse the row value instead of calling the method is_home again.

Estimate effort

Effort S

@MathieuLamiot
Copy link
Contributor

Sounds good. Just one quick though for the future: what if we add a third queue at some point? We would have to add another column to the datamodel.
It won't cost more and it would be more future-proof to add a column "queue_name" instead of "is_home", and have constant strings to identify the queues, like DEFAULT and HOME for now?

@MathieuLamiot
Copy link
Contributor

@CrochetFeve0251

@CrochetFeve0251
Copy link
Contributor

CrochetFeve0251 commented Oct 23, 2023

Sounds good. Just one quick though for the future: what if we add a third queue at some point? We would have to add another column to the datamodel. It won't cost more and it would be more future-proof to add a column "queue_name" instead of "is_home", and have constant strings to identify the queues, like DEFAULT and HOME for now?

@MathieuLamiot We could do this but we send a boolean is_home to the SAAS and not the name of the queue so I think that would be more logic to save a boolean.
'is_home' => $this->is_home( $url ),

@Miraeld
Copy link
Contributor

Miraeld commented Nov 23, 2023

As @Mai-Saad stated that the issue was not reproducible on 3.15.4, we concluded to block it for the moment.

@Miraeld Miraeld removed their assignment Nov 23, 2023
@piotrbak
Copy link
Contributor

Let's reopen it when needed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3rd party compatibility Issues related to 3rd party compatibility like theme, plugin or hosting effort: [XS] < 1 day of estimated development time module: remove unused css priority: low Issues that can wait type: enhancement Improvements that slightly enhance existing functionality and are fast to implement
Projects
None yet
8 participants