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

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

Merged
merged 4 commits into from
Sep 12, 2019

Conversation

epiqueras
Copy link
Contributor

@epiqueras epiqueras commented Sep 3, 2019

Reverts #16814

Resolves #17241

Description

This PR removes a filter that unsets auto-draft titles that was creating plugin compatibility issues and replaces it with client side code.

How has this been tested?

The test suites were ran and verified to still pass.

Types of Changes

Bug Fix: Don't unset auto-draft titles, because some plugins rely on it.

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • My code has proper inline documentation.
  • I've included developer documentation if appropriate.

@epiqueras epiqueras self-assigned this Sep 4, 2019
@epiqueras epiqueras added [Package] Core data /packages/core-data Backwards Compatibility Issues or PRs that impact backwards compatability and removed [Package] Editor /packages/editor labels Sep 4, 2019
@epiqueras epiqueras added this to the Gutenberg 6.4 milestone Sep 4, 2019
// 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.

@epiqueras epiqueras merged commit 53857f6 into master Sep 12, 2019
@epiqueras epiqueras deleted the fix/remove-auto-draft-filter branch September 12, 2019 18:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Backwards Compatibility Issues or PRs that impact backwards compatability [Package] Core data /packages/core-data
Projects
None yet
Development

Successfully merging this pull request may close these issues.

PR #16814 breaks Co-Authors-Plus plugin Guest Author creation functionality
3 participants