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

3.18 - Exclusions part #7069

Open
piotrbak opened this issue Oct 27, 2024 · 13 comments · Fixed by #7167
Open

3.18 - Exclusions part #7069

piotrbak opened this issue Oct 27, 2024 · 13 comments · Fixed by #7167
Assignees
Labels
effort: [S] 1-2 days of estimated development time type: enhancement Improvements that slightly enhance existing functionality and are fast to implement type: new feature Indicates the issue is a request for new functionality
Milestone

Comments

@piotrbak
Copy link
Contributor

User Story
As a user, I’d like to be able to exclude fonts from the feature using filter

Acceptance Criteria

  • We’ll introduce a filter and backend exclusion possibility
  • It’ll be possible to exclude the whole Google Fonts CSS file from being processed
  • The exclusion can be based either on argument or on the URL
  • The exclusion should be regex compatible
  • When excluded, the CSS file will not be processed and will remain in the HTML source
  • When excluded, fonts inside the CSS file will not be rewritten/downloaded
@piotrbak piotrbak added needs: r&d Needs research and development (R&D) before a solution can be proposed and scoped. type: enhancement Improvements that slightly enhance existing functionality and are fast to implement type: new feature Indicates the issue is a request for new functionality labels Oct 27, 2024
@piotrbak piotrbak added this to the 3.18 milestone Oct 27, 2024
@Khadreal
Copy link
Contributor

Khadreal commented Oct 30, 2024

This is dependent on the completion of #7063

Scope a solution:

We'll create a filter rocket_exclude_locally_host_fonts
Add a new method here

public function get_exclude_host_fonts() : array {
      $lists = $this->providers['defaultlists']->data_manager->get_lists();

       return $lists->host_fonts ?? [];
}

We add our new filter to the subscriber class
'rocket_exclude_locally_host_fonts' => 'add_host_fonts_exclusions'
And add_host_fonts_exclusions method in the same subscriber class

public function add_host_fonts_exclusions( $exclusion ) {
     return array_merge( (array) $exclusions, $this->dynamic_lists->get_exclude_host_fonts() );
}

In Frontend\Controller we would create a private method method which get exclusions and this method will be used in the rewriting process( created here #7063) to check if the rewrite should happen or not.
The exclusion should be compatible with regular expression.

private function get_exclusions(): array {
      return wpm_apply_filters_typed( 'string[]', 'rocket_exclude_locally_host_fonts', [] );
}

Estimation
[S]

@Khadreal Khadreal removed this from the 3.18 milestone Nov 5, 2024
@jeawhanlee
Copy link
Contributor

I think it doesn't matter whatever is passed to the filter, be it a full url or a part of it like the name, we should be able to exclude it using regex, we've done something similar in the past with other features, having 2 filters could seem redundant for accomplishing the same thing.

@jeawhanlee
Copy link
Contributor

I have a question, does the backend exclusion mentioned mean having a textarea in our settings page on the admin or rather handling externally, the same way we do with the dynamic lists @piotrbak @Khadreal @MathieuLamiot

@piotrbak
Copy link
Contributor Author

piotrbak commented Nov 6, 2024

@jeawhanlee It's only filter + backend for now

@jeawhanlee
Copy link
Contributor

@Khadreal We will not need to introduce a new textarea on the admin for exclusions as it will be backend for now.
CC @piotrbak

@Khadreal
Copy link
Contributor

Khadreal commented Nov 6, 2024

Thanks for spotting it @jeawhanlee
okay, I'll modify the grooming

@Khadreal
Copy link
Contributor

Khadreal commented Nov 6, 2024

@jeawhanlee the grooming has been updated.

@jeawhanlee
Copy link
Contributor

LGTM

@remyperona
Copy link
Contributor

The current grooming is missing to consider how should exclusions work when the Google fonts link is composed of multiple fonts?

Does it mean we need to rewrite the google link to keep the excluded font? So splitting the combine into 2 separate links, with only one being processed.

How does the exclusion interacts with the combine google fonts feature?

@piotrbak
Copy link
Contributor Author

piotrbak commented Nov 7, 2024

@remyperona What about excluding the font from being combined? Then it'll not be combined and might not be part of the rewrite system.

@remyperona
Copy link
Contributor

What about the case where the fonts are already combined, not by our feature?

@piotrbak
Copy link
Contributor Author

piotrbak commented Nov 8, 2024

Then, if our exclusion matches, we'll exclude whole combined (not by us) CSS file

@Khadreal Khadreal self-assigned this Nov 14, 2024
@Khadreal Khadreal added effort: [S] 1-2 days of estimated development time and removed needs: r&d Needs research and development (R&D) before a solution can be proposed and scoped. labels Nov 14, 2024
@piotrbak piotrbak added this to the 3.18 milestone Nov 28, 2024
@MathieuLamiot
Copy link
Contributor

MathieuLamiot commented Dec 2, 2024

Initially picked by @Khadreal but since he is off, I am moving this back to ToDo.
There is a commit to pick up from (see above)

@Miraeld Miraeld self-assigned this Dec 4, 2024
@Miraeld Miraeld linked a pull request Dec 4, 2024 that will close this issue
8 tasks
@hanna-meda hanna-meda self-assigned this Dec 9, 2024
@jeawhanlee jeawhanlee self-assigned this Dec 11, 2024
remyperona added a commit that referenced this issue Dec 13, 2024
Co-authored-by: Opeyemi Ibrahim <[email protected]>
Co-authored-by: Rémy Perona <[email protected]>
Co-authored-by: Michael Lee <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
effort: [S] 1-2 days of estimated development time type: enhancement Improvements that slightly enhance existing functionality and are fast to implement type: new feature Indicates the issue is a request for new functionality
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants