Export and access checks

Patches for 6.1 and 6.2 for the proposed change. This is adding a method to WebformSubmissionExporterInterface, so it's a BC break, Core has the 1:1 rule that allows to add methods to interfaces that have a 1:1 relationship to a class and implementations are expected to subclass from that, but also only does that in minor versions.

That part is a bit tricky, but this is IMHO the sensible way forward long-term. You could only commit this to 6.2, 6.1 users that had custom processes that relied on this not doing access checks in 6.1 and earlier like us will need to use a patch or other workaround or switch to 6.2.

Remove the extra empty line that snuck in the patches.

Also no tests yet, considering the number of times that these went back and forth on, that would be a good idea, didn't check if you already have tests for submission query access, if so then combining that with export tests should be doable.

I am open to committing the 6.2.x patch AS-IS.

The first patch #4 applied to 6.1.x branch

The second patch #4 applied to 6.2.x branch

Patch in #3345191-4: Export and access checks → applied to current 6.2.0-beta6 breaks drush wfx for me.
This seems related to the already mentioned 🐛 Webform 6.1.4 breaks drush data export Fixed .

I get infinite loops of

>  [notice] Exported 0 of 7 submissions…
>  [notice] Exported 0 of 7 submissions…
>  [notice] Exported 0 of 7 submissions…

with this patch applied. No further info when using --debug.

Ah, good find, thanks for testing. I didn't properly test that. The drush export also reuses the export controller and its batch methods and that was inconsistent. it had a hardcoded queryAccess(FALSE), so the total was always without query access while the actual export wasn't. I changed that to use getTotal(), I also refactored the whole patch to put the access check flag in export options, that is easier to pass around and makes more sense I think.

I also added a safety guard to that batch method, because even with consistent access checks, someone adding or removing a submission while you are batch-exporting could also cause an endless loop. I prefer progress < max over progress !== max in such a case, but didn't make that change.

Only adding a patch for 6.2.x for now, I don't think if you want to make that kind of change to 6.1.x anymore at this point.

The last submitted patch, 11: webform-export-access-6.2-3345191-11.patch, failed testing. View results →

Hm, that was based on an incomplete local patch. The test fail shows that access check needs to be further up, because there is a case with the range latest where the query gets cloned.

I can confirm my exports now work on 6.2.0-beta6 plus the patch in #3345191-13: Export and access checks → , thanks!

However, I cannot test/verify whether the actual issue at hand here is actually (still) fixed as I only partly understand the problem and do not have an existing problem caused by it which would be fixed (and neither do I use the domain module).

Our tests are working fine with the patch in #13. We did have to change our implementation of course, instead of calling accessCheck(FALSE), you need to pass it in as an exporter option, in case people were already relying on that.

Let's get this committed because it fixed the immediate issue

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