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.19 - Preload Fonts - Beacon insertion part #7240

Open
piotrbak opened this issue Jan 26, 2025 · 5 comments · Fixed by #7302
Open

3.19 - Preload Fonts - Beacon insertion part #7240

piotrbak opened this issue Jan 26, 2025 · 5 comments · Fixed by #7302
Assignees
Labels
effort: [S] 1-2 days of estimated development time
Milestone

Comments

@piotrbak
Copy link
Contributor

User Story
As a user, I’d like beacon to collect data when there’s no entry in the db

Acceptance Criteria

  • When visiting the uncached page while no data in DB, the same beacon script is added to process the preload fonts (no regression for beacon script injection)
@piotrbak piotrbak added this to the 3.19-prealpha milestone Jan 26, 2025
@piotrbak piotrbak changed the title 3.19 - Beacon insertion part 3.19 - Preload Fonts - Beacon insertion part Jan 26, 2025
@jeawhanlee
Copy link
Contributor

jeawhanlee commented Jan 31, 2025

Most of the part that handles this have already been introduced in 3.16 . We can find that module here We just need to declare the factory to be loaded there.

Scope a solution ✅

  • Create a Frontend Controller Class if not created: WP_Rocket\Engine\Media\PreloadFonts\Frontend\Controller to implement WP_Rocket\Engine\Common\PerformanceHints\Frontend\ControllerInterface;
  • Create 2 mandatory methods if not created ( optimize & add_custom_data ), to match the interface here and will focus on add_custom_data method that will hold custom data to be passed to the beacon during insertion, in this case system fonts which will filterable.
  • Create a Factory for PreloadFonts like the way we do here for ATF if not created
  • Create the queries method (product) required for this task if not created and make sure it extends WP_Rocket\Engine\Common\PerformanceHints\Database\Queries\AbstractQueries and implements ``WP_Rocket\Engine\Common\PerformanceHints\Database\Queries\QueriesInterface
  • Update service providers if not already having the factory

Estimate the effort ✅

[S]

@piotrbak piotrbak added the effort: [S] 1-2 days of estimated development time label Feb 6, 2025
@Miraeld Miraeld self-assigned this Feb 12, 2025
@Miraeld Miraeld linked a pull request Feb 12, 2025 that will close this issue
8 tasks
@Miraeld
Copy link
Contributor

Miraeld commented Feb 12, 2025

@wp-media/engineering-plugin-team I've created a PR related to this issue, however, as the branch feature/preload-fonts is missing the Database part of the new feature, I can't make it runnable. However, I've added abstract references in the ServiceProvider so references are there, but will need to modify later on.

use WP_Rocket\Engine\Common\PerformanceHints\Database\Table\AbstractTable as Table;
use WP_Rocket\Engine\Common\PerformanceHints\Database\Queries\AbstractQueries as Queries;

I'm wondering tho how would be the best way to handle this scenario... Should I base my own branch on #7242 even tho it's not validated yet ?

@Khadreal
Copy link
Contributor

I think you can base your branch off #7242 so you can validate what you've done properly

@Miraeld
Copy link
Contributor

Miraeld commented Feb 12, 2025

I was waiting your to get reviewed before doing this, to avoid changes in case there were some. But sure I'll do.

@Miraeld Miraeld removed a link to a pull request Feb 14, 2025
8 tasks
@Miraeld Miraeld linked a pull request Feb 14, 2025 that will close this issue
8 tasks
@hanna-meda hanna-meda self-assigned this Feb 19, 2025
Khadreal added a commit that referenced this issue Feb 19, 2025
Co-authored-by: Khadreal <opeyemi.khadri@gmail.com>
@wordpressfan
Copy link
Contributor

That's merged already

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
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants