Copied from #943772-190: field_delete_field() and others fail for inactive fields :

- I don't like the explicit "field_purge_batch()" call added after the "field_delete_field()" in forum_uninstall().
That's a convoluted DX pattern for module developers - "on uninstall, after deleting the fields you created on install, don't forget to run an explicit purge pass, so that users with few field data (e.g people just evaluating a module) can disable the field type modules right after that"

field_purge_batch(reasonable N) should be taken care of within field_delete_field(). Problem is, node type deletion (or, worse, uninstall of an entity type, = deletion of all bundles) can cascade to 10(0)s of field deletions, so we probably want to control that automatic purge by an optional param for field_delete_field().

Besides, the current field_purge_batch() call won't work as intended right now :
- If there are already fields/data pending deletion, there's no guarantee that this purge pass will clear the very data you want to clear. This would require an additional $field_name param for field_purge_batch() to clear the data from a specific field.
- If the field had data, completely clearing the data+instance+field requires at least *two* field_purge_batch() calls.
See the code logic over there : if the EFQ returns no result (all data has been purged), then wipe the instance and field.
Should be switched to : if the EFQ returns less results than the number we asked for, then wipe the instance and field.

Files: 
CommentFileSizeAuthor
#13 field_purge-1264756-13.patch11.8 KByched
FAILED: [[SimpleTest]]: [MySQL] 33,339 pass(es), 14 fail(s), and 13 exception(es).
[ View ]
#11 field_purge-1264756-11.patch11.67 KByched
FAILED: [[SimpleTest]]: [MySQL] 33,319 pass(es), 27 fail(s), and 26 exception(es).
[ View ]
#10 field_modules_enabled-1264728-9.patch4.35 KByched
PASSED: [[SimpleTest]]: [MySQL] 34,167 pass(es).
[ View ]
#9 field_modules_enabled-1264728-9.patch4.35 KByched
PASSED: [[SimpleTest]]: [MySQL] 34,173 pass(es).
[ View ]
#6 field_purge-1264756-6.patch10.7 KByched
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch field_purge-1264756-6.patch. This may be a -p0 (old style) patch, which is no longer supported by the testbots.
[ View ]
#5 field_purge-1264756-5.patch10.61 KByched
PASSED: [[SimpleTest]]: [MySQL] 32,937 pass(es).
[ View ]
#3 field_purge-1264756-3.patch10.47 KByched
PASSED: [[SimpleTest]]: [MySQL] 32,957 pass(es).
[ View ]
#1 field_purge-1264756-1.patch9.18 KByched
FAILED: [[SimpleTest]]: [MySQL] 32,924 pass(es), 10 fail(s), and 0 exception(es).
[ View ]

Comments

yched’s picture

Status:Active» Needs review
StatusFileSize
new9.18 KB
FAILED: [[SimpleTest]]: [MySQL] 32,924 pass(es), 10 fail(s), and 0 exception(es).
[ View ]

Tentative patch.

Status:Needs review» Needs work

The last submitted patch, field_purge-1264756-1.patch, failed testing.

yched’s picture

Title:Make field_purge_batch() work in one pass » Inline field purge within field_delete_field()
Status:Needs work» Needs review
StatusFileSize
new10.47 KB
PASSED: [[SimpleTest]]: [MySQL] 32,957 pass(es).
[ View ]

Should fix tests accordingly.

+ more explicit title.

catch’s picture

Subscribing.

yched’s picture

StatusFileSize
new10.61 KB
PASSED: [[SimpleTest]]: [MySQL] 32,937 pass(es).
[ View ]

#3 had a leftover phpdoc @todo.

yched’s picture

StatusFileSize
new10.7 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch field_purge-1264756-6.patch. This may be a -p0 (old style) patch, which is no longer supported by the testbots.
[ View ]

Rerolled after the move to /core.

sun’s picture

Issue tags:-needs backport to D7

#6: field_purge-1264756-6.patch queued for re-testing.

Status:Needs review» Needs work
Issue tags:+needs backport to D7

The last submitted patch, field_purge-1264756-6.patch, failed testing.

yched’s picture

Status:Needs work» Needs review
StatusFileSize
new4.35 KB
PASSED: [[SimpleTest]]: [MySQL] 34,173 pass(es).
[ View ]

Huh ? AFAICT #6 still applied with offset. Anyway, reroll.

yched’s picture

StatusFileSize
new4.35 KB
PASSED: [[SimpleTest]]: [MySQL] 34,167 pass(es).
[ View ]

Testbot stalled ? Reuploading.

yched’s picture

StatusFileSize
new11.67 KB
FAILED: [[SimpleTest]]: [MySQL] 33,319 pass(es), 27 fail(s), and 26 exception(es).
[ View ]

Oops, I rerolled the wrong patch, forget #9 and #10.
#6, rerolled.

Status:Needs review» Needs work

The last submitted patch, field_purge-1264756-11.patch, failed testing.

yched’s picture

Status:Needs work» Needs review
StatusFileSize
new11.8 KB
FAILED: [[SimpleTest]]: [MySQL] 33,339 pass(es), 14 fail(s), and 13 exception(es).
[ View ]

Should be better ?

Status:Needs review» Needs work

The last submitted patch, field_purge-1264756-13.patch, failed testing.

geerlingguy’s picture

I just spent a bit of time trying to figure out why fields weren't getting deleted by field_delete_field(), which led me down a rabbit hole to this issue. Would really love to see this implemented for better DX. (So, mostly just a +1 from me... will test if I can find some time).

andypost’s picture

does the issue still valid?

yched’s picture

Well, the APIs have changed quite a bit, but striclty speaking the interest is still there.

Although, on top of the cases mentioned in the OP where we don't want the mini-auto-purge to run (e.g deletion of whole bundles), we also don't want it to run during field deletion in config sync - since field purge is taken care of in a separate sync step.

So, maybe that's a lot of cases and the complexity might not be worth the DX gains.