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.

Sign up for GitHub

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

Jump to bottom

Compat: Remove filter that unsets auto-draft titles. #17317

Merged
merged 4 commits into from
Sep 12, 2019
Merged

Compat: Remove filter that unsets auto-draft titles. #17317

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
1068339
Compat: Remove filter that unsets auto-draft titles.
epiqueras Sep 3, 2019
b9b70cc
Core Data: Replace the auto draft filtering with client code.
epiqueras Sep 4, 2019
23b03da
Core Data: Update unit tests.
epiqueras Sep 4, 2019
96d550e
Core Data: Only apply auto draft logic to posts.
epiqueras Sep 12, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
30 changes: 0 additions & 30 deletions lib/compat.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,36 +30,6 @@ function gutenberg_safe_style_css_column_flex_basis( $attr ) {
}
add_filter( 'safe_style_css', 'gutenberg_safe_style_css_column_flex_basis' );

/**
* Filters inserted post data to unset any auto-draft assigned post title. The status
* of an auto-draft should be read from its `post_status` and not inferred via its
* title. A post with an explicit title should be created with draft status, not
* with auto-draft status. It will also update an existing post's status to draft if
* currently an auto-draft. This is intended to ensure that a post which is
* explicitly updated should no longer be subject to auto-draft purge.
*
* @see https://core.trac.wordpress.org/ticket/43316#comment:88
* @see https://core.trac.wordpress.org/ticket/43316#comment:89
*
* @param array $data An array of slashed post data.
* @param array $postarr An array of sanitized, but otherwise unmodified post
* data.
*
* @return array Filtered post data.
*/
function gutenberg_filter_wp_insert_post_data( $data, $postarr ) {
if ( 'auto-draft' === $postarr['post_status'] ) {
if ( ! empty( $postarr['ID'] ) ) {
$data['post_status'] = 'draft';
} else {
$data['post_title'] = '';
}
}
return $data;
}
add_filter( 'wp_insert_post_data', 'gutenberg_filter_wp_insert_post_data', 10, 2 );


/**
* Shim that hooks into `pre_render_block` so as to override `render_block`
* with a function that passes `render_callback` the block object as the
Expand Down
25 changes: 18 additions & 7 deletions packages/core-data/src/actions.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,6 +69,11 @@ export function addEntities( entities ) {
* @return {Object} Action object.
*/
export function receiveEntityRecords( kind, name, records, query, invalidateCache = false ) {
// Auto drafts should not have titles, but some plugins rely on them so we can't filter this
// on the server.
records = castArray( records ).map( ( record ) =>
record.status === 'auto-draft' ? { ...record, title: '' } : record
);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this bundles a post-specific behavior to a generic entity handler. Can't we instead apply the filter to the REST API endpoint only instead of a global PHP filter?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That would still break plugins that use the REST API and rely on the current behavior.

this bundles a post-specific behavior to a generic entity handler.

Yeah, it's unfortunate, but it won't break non-posts, it will just return the records.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought the breakage was related to the usage of a specific WordPress function (and not the bahavior itself).

Is there a clean way we can move this out of the entities or make it generic somehow?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought the breakage was related to the usage of a specific WordPress function (and not the bahavior itself).

Both

Is there a clean way we can move this out of the entities or make it generic somehow?

Not really, unless there is a way of adding plugin specific filters that I am not aware of.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we at least create an issue maybe to not forget to think about how we can make this generic. Maybe some functions on the entities config or something?

Also, there's something that feels wrong here. We don't want this behavior in a generic way server-side but we do want it in the client side (which can is a bit non-sense) (The entities abstraction is generic and not specific to the editor).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Like that?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That works for me. What bothers me is that we're harding this in the entities actions, while it's specific to the post entities only. Do you think there's any way to move this to the editor store instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, but I don't think we want that.

By having it here we enforce the correct behavior on all new client side code, and non-post entities are not affected.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we add a check to these two changes and ensure the "kind" is "postType"?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

96d550e

let action;
if ( query ) {
action = receiveQueriedItems( records, query );
Expand Down Expand Up @@ -235,18 +240,18 @@ export function* saveEntityRecord(
let error;
try {
const path = `${ entity.baseURL }${ recordId ? '/' + recordId : '' }`;
const persistedRecord = yield select(
'getRawEntityRecord',
kind,
name,
recordId
);

if ( isAutosave ) {
// Most of this autosave logic is very specific to posts.
// This is fine for now as it is the only supported autosave,
// but ideally this should all be handled in the back end,
// so the client just sends and receives objects.
const persistedRecord = yield select(
'getRawEntityRecord',
kind,
name,
recordId
);
const currentUser = yield select( 'getCurrentUser' );
const currentUserId = currentUser ? currentUser.id : undefined;
const autosavePost = yield select(
Expand Down Expand Up @@ -304,10 +309,16 @@ export function* saveEntityRecord(
yield receiveAutosaves( persistedRecord.id, updatedRecord );
}
} else {
// Auto drafts should be converted to drafts on explicit saves,
// but some plugins break with this behavior so we can't filter it on the server.
let data = record;
if ( persistedRecord.status === 'auto-draft' && ! data.status ) {
data = { ...data, status: 'draft' };
}
updatedRecord = yield apiFetch( {
path,
method: recordId ? 'PUT' : 'POST',
data: record,
data,
} );
yield receiveEntityRecords( kind, name, updatedRecord, undefined, true );
}
Expand Down
9 changes: 6 additions & 3 deletions packages/core-data/src/test/actions.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,8 @@ describe( 'saveEntityRecord', () => {
expect( fulfillment.next( entities ).value.type ).toBe(
'SAVE_ENTITY_RECORD_START'
);
const { value: apiFetchAction } = fulfillment.next();
expect( fulfillment.next().value.type ).toBe( 'SELECT' );
const { value: apiFetchAction } = fulfillment.next( {} );
expect( apiFetchAction.request ).toEqual( {
path: '/wp/v2/posts',
method: 'POST',
Expand Down Expand Up @@ -46,7 +47,8 @@ describe( 'saveEntityRecord', () => {
expect( fulfillment.next( entities ).value.type ).toBe(
'SAVE_ENTITY_RECORD_START'
);
const { value: apiFetchAction } = fulfillment.next();
expect( fulfillment.next().value.type ).toBe( 'SELECT' );
const { value: apiFetchAction } = fulfillment.next( {} );
expect( apiFetchAction.request ).toEqual( {
path: '/wp/v2/posts/10',
method: 'PUT',
Expand All @@ -68,7 +70,8 @@ describe( 'saveEntityRecord', () => {
expect( fulfillment.next( entities ).value.type ).toBe(
'SAVE_ENTITY_RECORD_START'
);
const { value: apiFetchAction } = fulfillment.next();
expect( fulfillment.next().value.type ).toBe( 'SELECT' );
const { value: apiFetchAction } = fulfillment.next( {} );
expect( apiFetchAction.request ).toEqual( {
path: '/wp/v2/types/page',
method: 'PUT',
Expand Down