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

Deleting a block with meta attribute does not delete value in database #4054

Closed
ghost opened this issue Dec 16, 2017 · 8 comments
Closed

Deleting a block with meta attribute does not delete value in database #4054

ghost opened this issue Dec 16, 2017 · 8 comments
Labels
[Feature] Block API API that allows to express the block paradigm. [Type] Enhancement A suggestion for improvement.

Comments

@ghost
Copy link

ghost commented Dec 16, 2017

Issue Overview

Deleting a block that contains attributes with source: 'meta' does not delete the corresponding row in wp_postmeta

Steps to Reproduce (for bugs)

  1. Create a block with an attribute = source meta
  2. Create a post containing such a block, fill in a value and save -> creates a row in wp_postmeta
  3. Delete the block and save. -> row in wp_postmeta still exists, leading to invalid output

Gutenberg 1.9.1

Expected Behavior

row in wp_postmeta is deleted

Current Behavior

row in wp_postmeta persists

@youknowriad
Copy link
Contributor

Do we really want to delete the meta value when deleting the block? I don't know the answer to this question.

It's still possible to provide a button to unset the value in the block itself setAttributes( { myattribute: null } )

@ghost
Copy link
Author

ghost commented Dec 18, 2017

Do we really want to delete the meta value when deleting the block?

In my opinion:

The fundamentalistic answer: Yes, of course. If a meta value is maintained via Gutenberg, there should never be a difference in what is in the database and what the user sees in the editor.

The pragmatic answer: Yes please, we might get in trouble otherwise.

The use case: The user can add a block "Contact Person" containing an email address, which is used in a 'the_content' filter to grab data from an external LDAP-Directory and display the info. That would still happen after the user deletes the block. (Although i can imagine some workaround, we should not rely on such weird stuff.)

The value is stored in meta to be used for queries, which also would show incorrect results. Or for mass updates in case the contact person changes.

It's still possible to provide a button to unset the value in the block itself

Get it, but does that work outside of a template with fixed blocks, when the user finally deletes the block?

PS.
I understand that it is unpredictable what devs do with meta values, so there may be a setting like
deleteMetaOnDeleteBlock: true/false ?

I also get that it's possible to hook into the rest api, analyze the payload and delete the row if no value is present. But i think that is way to complicated.

@greatislander
Copy link
Contributor

greatislander commented Dec 19, 2017

@youknowriad I would argue yes. If the idea is to manage post meta with blocks, the blocks should perform full CRUD operations on the relevant post meta values. This also makes the behaviour consistent with other blocks; if a block with content is deleted, the content is removed from the data store.

@greatislander
Copy link
Contributor

greatislander commented Jan 12, 2018

@youknowriad I'd like to bump this issue. If blocks are the new way to manage post meta they need to work the same way core meta fields currently work.

Step 1: Add Custom Field

screen recording 2018-01-12 at 02 04 pm

Step 2: Verify Presence of Post Meta

$ wp post meta get 1 custom
field

Step 3: Delete Custom Field

screen recording 2018-01-12 at 02 06 pm

Step 4: Verify Absence of Post Meta

$ wp post meta get 1 custom

No value is returned.

Adding a post meta block adds the meta; deleting it must delete the meta.

Edit: As per the suggestion:

It's still possible to provide a button to unset the value in the block itself setAttributes( { myattribute: null } )

What is the functional difference between this and deleting the block? We don't use a button in a Paragraph block to remove the text from the paragraph; if the block is deleted, the text is deleted.

@danielbachhuber
Copy link
Member

One idea: support a remove callback for block registration. This would let the block perform whatever data cleanup it needed.

I'm not sure how Gutenberg would know which metadata to programmatically remove when a block is removed.

@danielbachhuber danielbachhuber added [Type] Enhancement A suggestion for improvement. [Feature] Block API API that allows to express the block paradigm. labels Jan 23, 2018
@ghost
Copy link
Author

ghost commented Jan 24, 2018

I'm not sure how Gutenberg would know which metadata to programmatically remove when a block is removed.

Sorry if i am not up to date as i left the WordPress world in the meantime, but isn't there the possibility to define block attributes server-side? At the first glance i would be fine to require that as soon as a meta attribute is defined, so Gutenberg has the information it needs to delete the value.

Callback: also welcome, but just brainstorming a little bit:

More advanced applications may want to go beyond that limited meta attribute thing, so it would be great to register a class which optionally performs all CRUD-Methods, eg. storing data in custom tables, as serialized objects, linking to taxonomies, whatever.

And cleaning up.

Acting like a model component in a MVC paradigm. Could also do validations, which could be the server side API for #4063.

(Actually that would also be the correct place for the attribute definitions)

@youknowriad
Copy link
Contributor

youknowriad commented Mar 6, 2018

I'm not sure how Gutenberg would know which metadata to programmatically remove when a block is removed.

We can know this but I think I disagree with this issue because the same meta attribute could be used in several blocks, so deleting a block should not delete the meta attribute, the same way deleting a meta box from ACF doesn't delete the meta value.

@youknowriad
Copy link
Contributor

I'm closing this for now as we approach merge proposal, we need to triage our issues but let's keep discussing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Block API API that allows to express the block paradigm. [Type] Enhancement A suggestion for improvement.
Projects
None yet
Development

No branches or pull requests

3 participants