WordPress / wordpress-playground

Run WordPress in the browser via WebAssembly PHP
https://w.org/playground/
GNU General Public License v2.0
1.64k stars 258 forks source link

Remove the step "importThemeStarterContent" #1994

Open bph opened 1 day ago

bph commented 1 day ago

The documentation lists a step: "importThemeStarterContent" Implementation issue shows, that it's an option on the InstallTheme step.

It's already included in the installTheme step. documentation

Example quoted from the PR:

{
  "steps": [
    {
      "step":"installTheme",
      "themeZipFile":{
        "resource":"wordpress.org/themes",
        "slug":"twentytwenty"
       },
       "options":{
         "importStarterContent": true
       }
    }
  ]
}
bph commented 1 day ago

There are two files:

I can't figure out where to make the updates to remove the step from the documentation, though.

bph commented 1 day ago

And maybe it shouldn't be under steps at all but added to the InstallTheme step..

I see in import-theme-starter-content.ts

/**
 * @inheritDoc importThemeStarterContent
 * @example
 *
 * <code>
 * {
 *      "step": "importThemeStarterContent"
 * }
 * </code>
 */

and further down

    step: 'importThemeStarterContent';
    /**
     * The name of the theme to import content from.
     */
    themeSlug?: string;
}

/**
 * Imports a theme Starter Content into WordPress.
 *
 * @param playground Playground client.
 */

Should that be all that needs to be deleted?

bgrgicak commented 1 day ago

@bph importThemeStarterContent is both a step and an option in the installTheme step. Why would you like to remove the step?

It's nice to have both in case you need to do something more custom. For example, you can install a theme that doesn't have starter content, add it using a writeFile step, and run importThemeStarterContent later.

bgrgicak commented 1 day ago

There are two files:

* packages/playground/blueprints/src/lib/steps/import-theme-starter-content.ts

* packages/playground/blueprints/src/lib/steps/import-theme-starter-content.spec.ts

I can't figure out where to make the updates to remove the step from the documentation, though.

You would update the import-theme-starter-content.ts file to make documentation example changes. The /import-theme-starter-content.spec.ts file only contains unit tests that automatically check if the step works.

bgrgicak commented 1 day ago

To answer your question about deleting steps.

Check if the step is used

We collect step usage stats in Google Analytics, so before we remove anything, we can check how frequently it is used. The importThemeStarterContent step is rarely used.

Before we can remove it, we usually mark it as deprecated for a while before actually removing it.

Deleting a step

To delete a step you need to delete the step interface, in this case, it would be this code: https://github.com/WordPress/wordpress-playground/blob/f677792cce1a664f72ec9691769b7cd6a4a04f60/packages/playground/blueprints/src/lib/steps/import-theme-starter-content.ts#L4-L21

You also need to delete any references to the interface by searching the Playground project for its name (i.e. ImportThemeStarterContentStep).

Finally, you need to remove the step from the blueprint schema, in this case, it would be this code: https://github.com/WordPress/wordpress-playground/blob/07fb8bac24f02db0c40a2f465597a05021c01d7e/packages/playground/blueprints/public/blueprint-schema.json#L556-L583

bph commented 1 day ago

Thank you, @bgrgicak Ah sorry, this wasn't clear. For this issue, I was only thinking of removing the step from the Documentation, not so much from Playground.

This PR #1521 was the only one I found about this, and it only mentions the installTheme option, unless I am missing something. There is also no example on how to use the step variation. My conclusion was that the documentation wasn't updated, and we can take care of it by removing it from the Documentation. Only when I tried to find the spot where the documentation originated, I found the step files, and was confused.

Before we can remove it, we usually mark it as deprecated for a while before actually removing it.

This was a misunderstanding, and I would love to update the documentation with a working example for the step variation on how to do this in a blueprint. The current version doesn't help here much. Or I don't understand it.

For example, you can install a theme that doesn't have starter content, add it using a writeFile step, and run importThemeStarterContent later.

That's a great use case. How would this be accomplished via blueprint?

bgrgicak commented 12 hours ago

The current version doesn't help here much. Or I don't understand it.

I agree, some of our Blueprint examples are so minimal that they don't do anything on their own or even break the site.

bgrgicak commented 12 hours ago

That's a great use case. How would this be accomplished via blueprint?

Here is a minimal example which adds a new homepage section.

Image