Edits to templates and template parts inside subfolders are not saved correctly

carolinan commented 1 year ago

Description

Block themes can place template parts in subfolders inside the parts folder. (See previous issue: https://github.com/WordPress/gutenberg/issues/20375)

Changes made to these template parts are not saved as a user customization to that part. Instead, it looks like a new template part is created, and there is something wrong with the saved state: the changes to the customized part show as not being saved.

Step-by-step reproduction instructions

Activate Twenty Twenty-Three.

Inside the parts folder, create a new folder called "headers". Move header.html inside this folder.

In home.html, change  to 

Confirm that the header template part loads correctly. In the editor, select the header template part and make some changes. Either directly on the home template screen or in isolation -it doesn't matter. Save. Notice that the save seems complete, and the "Site updated" notification appears. But the Save button is still in the state that shows that there are unsaved changes, and the site preview button next to it is disabled.

Click on the Icon in the top left corner of the Site Editor to open the navigation sidebar (the dark grey sidebar on the left side) Confirm that the save button at the bottom of the sidebar is showing that there are unsaved changes. Click on this save button. Notice that the notification "Site update" shows up but the state doesn't change; it still shows that there are unsaved changes.

Using the sidebar, please navigate to the "All template parts" screen to view the list of template parts. Notice that the headers/header template part added by the theme is not labeled as being customized. Instead, there are multiple user-created template parts with the name headers//header in the list.

List of template parts that show the incorrect template parts

Screenshots, screen recording, code snippet

(I only have mobile internet at the moment, I will upload a video later.)

Environment info

Tested on macOS, nginx, PHP 8.0.22 using: WordPress 6.3.1 with and without Gutenberg 16.6.0. WordPress 6.2.2

I did not test on versions older than 6.2.2.

Please confirm that you have searched existing issues in the repo.

Yes

Please confirm that you have tested with all plugins deactivated except Gutenberg.

Yes

carolinan commented 1 year ago

The same problem happens for templates, not just parts.

annezazu commented 1 year ago

Adding this to 6.4 for triage.

getdave commented 12 months ago

Tested this and confirmed it is a bug.

It seems that when editing in focus mode the changes are saved.

However, when editing in the editor the changes are dispatched to the REST API...

....but when reloading the Editor the Header returns back to the original state without any changes you made. It's like it's loading the wrong part. Indeed the GET request to /wp/v2/template-parts/emptytheme//headers/header returns the content from the uncustomized template part (i.e. it doesn't have the changes I made the template part despite my just having saved and seen the REST POST request successfully dispatched).

It seems that if you in the Site View sidebar under Patterns -> Template Part the headers/header tempalte part gets duplicated over and over. Whereas if you make change to a template that is not nested then the original template part shows up as having "Customizations".

What I expected to happen is that there would only be a single Header template part and it would show "Customizations" given that I'd made efforts and saved those.

All this leads me to believe the REST API might not know how to resolve edits for customized template parts for subfolders. I'm yet to confirm this assumption however.

getdave commented 12 months ago

Looking into this in more detail...

I made a change to the "Header" template part to add a paragraph block.
I confirmed a POST request is dispatched and a record is created in the database with the correct content.
I reloaded the editor.
There is a GET request to fetch the "Header" template part - wp/v2/template-parts/emptytheme//headers/header. The "id" of the template part is headers/header with a / as the delineator. This request returns the "Header" template part from the file system and not the one from the database.
I then made the exact same request using @wordpress/core-data but this time I changed the "id" to be in the format headers-header with a - as the delineator.
```
wp.data.select( 'core' ).getEntityRecord(
'postType',
'wp_template_part',
'emptytheme//headers-header',
```

)

- This request returns the template part record _from the database_ and _not_ the one from the filesystem.

This means that you might think that one way to fix this is to convert any slashes within the "id" (aka `slug`) into hyphens. This will cause the request to the REST API to dispatch as we did manually above with Core Data. Such a change can be made as follows inside `packages/block-library/src/template-part/edit/utils/create-template-part-id.js`:

```js
export function createTemplatePartId( theme, slug ) {
        // Replace slashes with hyphens.
    slug = slug.replace( /\//g, '-' );

    return theme && slug ? theme + '//' + slug : null;
}

Implementing this does fix the immediate problem in that the editor will now load the template part from the database and not from the file system.

However, the problem is that template part slugs can themselves contain hyphens. I need to test what happens if our template part is called headers//header-primary as presumably this would be converted to headers-header-primary and the REST API would have no means to know how to distinguish the slug from the nested directory.

I tested this by:

creating a template on the filesystem in parts/headers/header-primary.html.
loading the editor the content is loaded correctly from the filesystem.
i made changes to the template part in the Editor to add a paragraph block and saved the changes.
Confirmed that record is created in the database.
Reloaded the Editor.
The Editor shows the old contents from the template part on the filesystem without any of my changes.
However...I then made a request in DevTools Console using Core Data as follows (notice how I am converting all / in the slug into -):
```
wp.data.select( 'core' ).getEntityRecord(
'postType',
'wp_template_part',
'emptytheme//headers-header-primary',
```

)


- This resulted in the record from the database bring returned!

So in theory the fix proposed above to replace `/` in the slug with `-` does work. 

## Bugs

However it looks like there are a few bugs - for example going to `Site View -> Patterns -> Template Parts` reveals two items for the nested header pattern:

<img width="1027" alt="Screenshot 2023-09-27 at 20 47 59" src="https://github.com/WordPress/gutenberg/assets/444434/5e0ea064-4724-4543-b654-553d310bae11">

This may be because there should be some relationship between the template part on the filesystem and the one in the database which is missing? Or it might be a quirk in the query/display logic of this area of the interface.

Also the title of the template part in the database has a title with _two_ slashes representing the subdirectory `header//header-primary` rather than the single slash that is used for the template part from the filesystem.

getdave commented 12 months ago

I'll be working on this here. If anyone would like to jump in please do:

https://github.com/WordPress/gutenberg/pull/54889

Further updates will be posted there.

carolinan commented 12 months ago

@getdave Thank you for the thorough testing and report.

getdave commented 12 months ago

Summary of Problem

TL;DR

WordPress slugs (post_name) cannot contain slashes.
Therefore it is impossible to map Template Part Post slugs to file system paths.
This prevents you from editing Template Parts that are within subdirectories using the Editor, and also causes "orphaned" Post Records to be created in the database.

Recommendation: for now official documentation should be updated to advise Theme authors that they cannot use subdirectories within the parts/ directory to store Template Parts. A more fundamental fix is required if we wish to enable this functionality.

More detail

Template Parts (abbreviated to TP from here) exist on the filesystem within the Theme's /parts directory. When edits are made to a TP in the Editor, a custom post type of type wp_template_part is created in the database to store those edits.
When the CPT is saved, the slug (stored as post_name field) should match the template part's path on the filesystem (e.g. header/header-primary).
However...WordPress sanitises the slug which converts slashes to hyphens (e.g. header-header-primary).
This means that when the Editor looks for an edited template part record, it cannot find one. This is because the REST API request looks for a CPT with a slug of header/header-primary but this does not exist. The record in the database has a slug (aka post_name) of header-header-primary (for reasons outlined above).
The result is that you can save edits to a template part in the Editor, but once you reload then those edits are no longer shown. They still exist in the database, but the Editor cannot find them and thus falls back to the template part on the filesystem.
In addition if you go to the Site Editor "Site View" (left sidebar) and then to Patterns -> Template Parts you will see duplicate entries for any template part that uses a subdirectory. This is because...
...the REST API collection endpoint for Templates (i.e. get_items method) will return "duplicate" entries for a given template part if it is nested in a subdirectory. These duplicates are:
- TP from filesystem with slug header/header-primary.
- TP Post record from database with slug header-header-primary
This because the REST endpoint cannot use the slug to marry the TP record in the database with the TP on the filesystem. Why?
- get_items is called to fetch all TPs
- get_block_templates is called to fetch all the templates parts. It should de-dupe the results to ensure that if a template part has been customised then only the record from the database is returned and not the one from the filesystem.
- TP Posts are mapped to WP_Block_Template instances in _build_block_template_result_from_post. During this process the has_theme_file property is set to false. It should be set to true because this TP is based upon a TP on the filesystem.
- this is because inside of _get_block_template_file the path to the template part on the filesystem is constructed using the raw slug. As the slug has been sanitized to strip all slashes (i.e. directory paths) it does not map to the matching template part on the filesystem (e.g. wp-content/themes/emptytheme/parts/headers-header-primary.html when it needs to be wp-content/themes/emptytheme/parts/headers/header-primary.html).
- Back in get_block_templates we now have a list of TP Posts from the database. We next need to fetch any TPs from the filesystem. This process should omit any TPs that have a corresponding Post record.
- The work happens in _get_block_templates_files.
- However, this does not work as we'd expect because it is based on matching against slugs (which as we know do not match!).
- As a result both the TP Post and the TP from the filesystem get included in the final result returned from the REST API.

Suggested Fix

I wonder whether we ought to abandon the idea of mapping filesystem path with template part post slug?

Why?

we know that WP does not allow post_name to include slashes (and what Ben said below). This means we cannot use another character to "represent" a directory path.
once we save a Post with a slug using hypens to replace the original directory path's slashes, we cannot then reliably reverse engineer the file path from the slug.

Both of the above mean we cannot rely on slug to map to filesystem for nested paths. As a result we cannot reliably dedupe customized template parts from filesystem template parts.

Therefore I propose we consider utilising post meta to store the original template part filesystem path. My understanding is that Post Meta is well suited to store information such as this. With this information stored against the TP Post record we can:

create a Template Part Post by converting all slashes to hyphens and storing that as slug whilst preserving the file path of the original template part in post meta.
look up a given Template Part by hyphenated slug as this will map directly to the record in DB.
reliably ensure that only a single record is returned for each Template Part when querying the REST API for template parts using the collection endpoint. This is because we will be able to utilise the filesystem path stored in the Post record's post meta to determine whether it matches with an existing record on the filesystem.

If we do this we should consider the findings about using slashes in Post Meta in this Trac ticket which I commented about.

carolinan commented 12 months ago

By chance, Sergey mentioned this trac ticket in a meeting this morning https://core.trac.wordpress.org/ticket/58132

scruffian commented 12 months ago

One idea to resolve this might be to use a different character instead of a / as a delimiter in the slug field. This will not work because:

a slug consists of solely lowercase alphanumeric characters, dashes and underscores, without 2 or more dashes in a row (sequences of underscores are allowed). Furthermore, a slug cannot start or end with a hyphen.

Any character we use as a delimiter can also be used on the file system, so it can't be safely assumed to be a delimiter.

getdave commented 12 months ago

By chance, Sergey mentioned this trac ticket in a meeting this morning https://core.trac.wordpress.org/ticket/58132

Yes. That is interesting because that notes that using slashes in Post Meta can cause problems on some environments. So again we'd need to store some other known character in the meta to stand in for/represent a slash and then reverse engineer that when we want to get the "real" path for deduping.

annezazu commented 7 months ago

After a review by core editor triage leads and core editor tech leads, this has been removed from the board for 6.5 consideration.

jeremyfelt commented 7 months ago

Tagging my previous #49100 as related. I'll close that as a duplicate because this ticket has activity.

WordPress / gutenberg