#2503047: Migrate the D6/D7 actions table to D8 has turned up a sneaky little bug -- node.schema.yml misdefines the configuration of the node_unpublish_by_keyword_action plugin. The result is that you can't save an action which uses this plugin -- it will always throw a config schema exception.

Proposed Resolution

Make node.schema.yml to match the plugin's configuration.

CommentFileSizeAuthor
#12 2578519-12.patch1.57 KBphenaproxima
#12 2578519-12-FAIL.patch1.07 KBphenaproxima
node-unpublish-action-schema.patch510 bytesphenaproxima

Comments

phenaproxima created an issue. See original summary.

phenaproxima’s picture

phenaproxima
he/him
Primary language English
Location Massachusetts
commented
Status: Active » Needs review
moshe weitzman’s picture

moshe weitzman commented
Status: Needs review » Reviewed & tested by the community

In other words, virtually noone has tried to use since config schema went in. Migrating Actions is quite the luxury IMO.

Wait for green before commit.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, node-unpublish-action-schema.patch, failed testing.

Status: Needs work » Needs review

phenaproxima queued node-unpublish-action-schema.patch for re-testing.

phenaproxima’s picture

phenaproxima
he/him
Primary language English
Location Massachusetts
commented
Status: Needs review » Reviewed & tested by the community

I think PIFR knows that it's about to go extinct, so it's throwing a lot of tantrums. Restoring RTBC.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, node-unpublish-action-schema.patch, failed testing.

Status: Needs work » Needs review

phenaproxima queued node-unpublish-action-schema.patch for re-testing.

Status: Needs review » Needs work

The last submitted patch, node-unpublish-action-schema.patch, failed testing.

phenaproxima’s picture

phenaproxima
he/him
Primary language English
Location Massachusetts
commented
Status: Needs work » Reviewed & tested by the community

PIFR keeps experiencing a segmentation fault in the same test, which has absolutely nothing to do with this patch. Utter nonsense.

DrupalCI passes the patch with no problems. Seeing as how PIFR is about to be decommissioned, restoring RTBC.

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
commented
Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs tests, +Needs upgrade path

I believe this needs an upgrade path for existing sites, else their schema will get out of sync. See docs at https://www.drupal.org/node/2535316.

We also seem to be lacking any sort of test coverage for this plugin, because this obviously doesn't currently work at all with the incorrect name. :\

phenaproxima’s picture

phenaproxima
he/him
Primary language English
Location Massachusetts
commented
Status: Needs work » Needs review
Issue tags: -Needs tests
StatusFileSize
new 2578519-12-FAIL.patch1.07 KB
new 2578519-12.patch1.57 KB

Now with a test!

The last submitted patch, 12: 2578519-12-FAIL.patch, failed testing.

The last submitted patch, 12: 2578519-12-FAIL.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 12: 2578519-12.patch, failed testing.

Status: Needs work » Needs review

phenaproxima queued 12: 2578519-12.patch for re-testing.

Kazanir’s picture

Kazanir commented
Status: Needs review » Reviewed & tested by the community

This looks good; the test demonstrates that no one can have ever saved one of these (due to the schema mismatch) so we'd be adding an update function for actions that can't actually have ever existed.

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
commented
Status: Reviewed & tested by the community » Needs work

I believe we still need an upgrade path for existing sites, no?

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Needs work » Reviewed & tested by the community
Issue tags: -Needs upgrade path

We don't need an upgrade patch - schema are static and parsed once the caches are flushed. The reason we didn't detect this in HEAD is that was nothing was using the schema. Test coverage is added - nice work.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Reviewed & tested by the community » Fixed

Committed 20fe22a and pushed to 8.0.x. Thanks!

  • alexpott committed 20fe22a on 
    Issue #2578519 by phenaproxima, webchick: Node has invalid config schema...
webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
commented

Oh wow, didn't realize. Great!

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.