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

Refactor/font library api #56993

Closed
wants to merge 22 commits into from
Closed

Refactor/font library api #56993

wants to merge 22 commits into from

Conversation

pbking
Copy link
Contributor

@pbking pbking commented Dec 12, 2023

(WIP)

Refactoring Font Library API

This change implements the Font Library API in the following way:

GET /wp/v2/font-collections/
GET /wp/v2/font-collections/<slug>

GET /wp/v2/font-families
GET /wp/v2/font-families/<id>
POST /wp/v2/font-families
PUT /wp/v2/font-families/<id>
PATCH /wp/v2/font-families/<id>
DELETE /wp/v2/font-families/<id>

Note: The process of Creating a Font Family with font face data is done with a POST request. If the slug matches a Font Family already installed the collection of that Font Family will be PATCHED with the incoming Font Family/Font Face information.

For example:

POST /wp/v2/font-families

{
  //we dont' know the id, it may or might not already exists.  
  "font_family_data": {
    "slug": "slug",
    "family": "Family",
     "fontFamily": "FontFamily"
     "fontFace": [{
        "fontFamily": "FontFamily",
        "fontStyle": "normal",
        "fontWeight": "400",
        "src": "http://someurl.com/font.ttf"
      }]
  }
}

And you'll get back the Font Face object. Just like what you sent, but with an id.

{
  "id": 123,
  "font_family_data": {
    "slug": "slug",
    "family": "Family",
     "fontFamily": "FontFamily"
     "fontFace": [{
        "fontFamily": "FontFamily",
        "fontStyle": "normal",
        "fontWeight": "400",
        "src": "http://someurl.com/font.ttf"
      }]
  }
}

Do it again some other time and you just add font faces to the font family:

POST /wp/v2/font-families

{
  //we dont' know the id, it may or might not already exists.  
  "font_family_data": {
    "slug": "slug",
    "family": "Family",
     "fontFamily": "FontFamily"
     "fontFace": [
        "fontFamily": "FontFamily",
        "fontStyle": "italic",
        "fontWeight": "400",
        "src": "http://someurl.com/italic_font.ttf"
      ]
  }
}

//It's added to the collection. We're doing a PATCH operation.

Response

{
 "id": 123,
 "font_family_data": {
   "slug": "slug",
   "family": "Family",
    "fontFamily": "FontFamily"
    "fontFace": [{
       "fontFamily": "FontFamily",
       "fontStyle": "normal",
       "fontWeight": "400",
       "src": "http://someurl.com/font.ttf"
     }, {
       "fontFamily": "FontFamily",
       "fontStyle": "italic",
       "fontWeight": "400",
       "src": "http://someurl.com/italic_font.ttf"
     ]
 }
}

Now if instead we want to remove one of them but keep another we make a PUT operation. Only those elements that are included in this payload stick around.

PUT /wp/v2/font-families/123

{
  "id": 123,
  "font_family_data": {
    "slug": "slug",
    "family": "Family",
     "fontFamily": "FontFamily"
     "fontFace": [{
        "fontFamily": "FontFamily",
        "fontStyle": "normal",
        "fontWeight": "400",
        "src": "http://someurl.com/font.ttf"
      }]
  }
}

Now we get it back with exactly the number of fontFaces that we sent it. We're not appending.

Response:

{
  "id": 123,
  "font_family_data": {
    "slug": "slug",
    "family": "Family",
     "fontFamily": "FontFamily"
     "fontFace": [{
        "fontFamily": "FontFamily",
        "fontStyle": "normal",
        "fontWeight": "400",
        "src": "http://someurl.com/font.ttf"
      }]
  }
}

Resource operations

There are THREE ways resources can be managed. The end result in all of these situations is that the Font Face contains a src property that points to a URL of the file asset.

  • The font face points to a hosted asset (there's nothing to do when this is the case)
  • The font face points to a hosted asset, but we want that hosted asset to be downloaded and installed locally.
  • The font face points to assets from the user's machine and we want to copy those locally.

Options to represent the action needed to take:

"presence of files" : If files are present in the upload payload we can assume that we want to operate on provided files and will leverage those assets. This doesn't take care of the other to actions though. We're not doing this but I think we should no mater how we discern how to handle the other two situations.

"another property": Instead of a 'src' property a different property is used to denote different actions ("uploadedFile" or "downloadFromUrl"). The resource would be process and the "src" property would be set and the special attribute removed.

"action attribute": Include ?action=install or some other attribute in the PUT/POST/PATCH calls. This would then either copy the resource or not based on the value (or presence?) of that attribute.

PUT /wp/v2/font-families/123?action=install

"special string": If a src has a special value such as 'download()' in the value of the request the asset would be copied and then a regular "src" value would be written:

    REQUEST      "src": "download(http://someurl.com/font.ttf)"
    RESPONSE    "src": "http://myownurl.com/font.ttf"

Kind of like "another property" but keeping it in the value so that the shape of the object doesn't change.

"another property" is how it was (and still currently is) implemented. "special string" is what I liked best until I typed out all of the above but "action attribute" is what I like best and seems the most RESTful.

@pbking pbking requested a review from spacedmonkey as a code owner December 12, 2023 19:18
@pbking pbking marked this pull request as draft December 12, 2023 19:18
Copy link

Warning: Type of PR label mismatch

To merge this PR, it requires exactly 1 label indicating the type of PR. Other labels are optional and not being checked here.

  • Type-related labels to choose from: [Type] Automated Testing, [Type] Breaking Change, [Type] Bug, [Type] Build Tooling, [Type] Code Quality, [Type] Copy, [Type] Developer Documentation, [Type] Enhancement, [Type] Experimental, [Type] Feature, [Type] New API, [Type] Task, [Type] Performance, [Type] Project Management, [Type] Regression, [Type] Security, [Type] WP Core Ticket, Backport from WordPress Core.
  • Labels found: .

Read more about Type labels in Gutenberg. Don't worry if you don't have the required permissions to add labels; the PR reviewer should be able to help with the task.

Copy link

github-actions bot commented Dec 12, 2023

This pull request has changed or added PHP files. Please confirm whether these changes need to be synced to WordPress Core, and therefore featured in the next release of WordPress.

If so, it is recommended to create a new Trac ticket and submit a pull request to the WordPress Core Github repository soon after this pull request is merged.

If you're unsure, you can always ask for help in the #core-editor channel in WordPress Slack.

Thank you! ❤️

View changed files
❔ lib/experimental/fonts/font-library/class-wp-rest-font-collection-controller.php
❔ lib/experimental/fonts/font-library/class-wp-rest-font-family-controller.php
❔ phpunit/tests/fonts/font-library/wpFontCollectionController/wpRestFontCollectionController.php
❔ phpunit/tests/fonts/font-library/wpRestFontLibraryController/create_item.php
❔ phpunit/tests/fonts/font-library/wpRestFontLibraryController/delete_item.php
❔ phpunit/tests/fonts/font-library/wpRestFontLibraryController/get_item.php
❔ phpunit/tests/fonts/font-library/wpRestFontLibraryController/get_items.php
❔ phpunit/tests/fonts/font-library/wpRestFontLibraryController/patch_item.php
❔ phpunit/tests/fonts/font-library/wpRestFontLibraryController/update_item.php
❔ lib/experimental/fonts/font-library/class-wp-font-collection.php
❔ lib/experimental/fonts/font-library/class-wp-font-family.php
❔ lib/experimental/fonts/font-library/class-wp-font-library.php
❔ lib/experimental/fonts/font-library/font-library.php
❔ lib/load.php
❔ phpunit/tests/fonts/font-library/wpRestFontLibraryController/base.php

Copy link

github-actions bot commented Dec 12, 2023

Flaky tests detected in 53e9001.
Some tests passed with failed attempts. The failures may not be related to this commit but are still reported for visibility. See the documentation for more information.

🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/7224711914
📝 Reported issues:

@creativecoder
Copy link
Contributor

creativecoder commented Dec 16, 2023

POST /wp/v2/font-families
PUT /wp/v2/font-families/[id]
PATCH /wp/v2/font-families/[id]

Note that the WP_REST_Server doesn't differentiate between the POST, PUT, and PATCH methods, and none of the existing Core endpoints use them separately.

I think we should simplify to

POST /wp/v2/font-families (create)
POST /wp/v2/font-families/[id] (update)

@creativecoder
Copy link
Contributor

"special string": If a src has a special value such as 'download()' in the value of the request the asset would be copied and then a regular "src" value would be written:

This seems to "magical" to me.

  • Downloading a file should be an explicit action, not implied by using a particular syntax.
  • Updating the font family should be a literal operation, what you provide as the font family data in the request is what's saved in the database.

@pbking
Copy link
Contributor Author

pbking commented Dec 18, 2023

This seems to "magical" to me.

Yeah, I agree. I liked the magic for a minute, but I agree that an action flag (or maybe an "copy=true" boolean flag) is superior.

@pbking
Copy link
Contributor Author

pbking commented Dec 18, 2023

I think we should simplify to
POST /wp/v2/font-families (create)
POST /wp/v2/font-families/[id] (update)

One of the most useful actions for this will bee to update a Font Family without knowing the ID. Which I think we can still do with just those endpoints but would look like this:

POST /wp/v2/font-families (create, patch)
POST /wp/v2/font-families/[id] (update)

Maybe I'm overthinking how advantageous that action is but I think it's important and helpful to provide that.

@pbking
Copy link
Contributor Author

pbking commented Jan 17, 2024

closing in leu of #57688

@pbking pbking closed this Jan 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants