Contrib.social

Reset highwater mark *before* rolling back

Open on Drupal.org →
Created on 15 September 2016, over 8 years ago
Updated 2 March 2025, about 2 months ago

Follow-up to #2485385: Move highwater field support to the source plugin, and do not expose its internals on MigrationInterface →

Problem/Motivation

Per alexpott's comment → :

Whilst reviewing this patch I pondered if we could set the highwater properly during rollback (see #125 and #127). After thinking about @mikeryan's response some more I think we should more the NULL setting from postRollback() to preRollback() this means that is the rollback fatals for any reason then the highwater is correctly indeterminate rather than wrong.

Good point - worst-case scenario setting it in preRollback(), you have to reimport everything if rollback fails. Worst-case scenario in postRollback() (as it is now), reimporting misses anything that was successfully rolledback - i.e., data loss.

Proposed resolution

Move the highwater code in SourcePluginBase from postRollback() to preRollback().

Remaining tasks

Implement the solution (Novice task).
Tests?

User interface changes

N/A

API changes

None

🐛 Bug report
Status

Needs work

Version

11.0 🔥

Component

migration system

Created by

🇺🇸United States mikeryan Murphysboro, IL, USA

Live updates comments and jobs are added and updated live.
  • Needs tests

    The change is currently missing an automated test that fails when run with the original code, and succeeds when the bug has been fixed.

Sign in to follow issues

Merge Requests

Comments & Activities

Not all content is available!

It's likely this issue predates Contrib.social: some issue and comment data are missing.

  • First commit to issue fork.
  • Merge request !11340Reset highwater mark *before* rolling back → (Open) created by dcam
  • Pipeline finished with Failed
    about 2 months ago
    Total: 99s
    #437770
  • Pipeline finished with Success
    about 2 months ago
    Total: 778s
    #437774
  • Comment about 2 months ago →
    🇺🇸United States dcam

    I didn't follow the recommended test plan. I created a unit test instead. Let me know if that isn't acceptable.

  • Comment 7 days ago →
    🇺🇸United States smustgrave 
    1) Drupal\Tests\migrate\Unit\MigrateSourceTest::testPreRollback
Expectation failed for method name is "set" when invoked 1 time.
Method was expected to be called 1 time, actually called 0 times.
FAILURES!
Tests: 13, Assertions: 37, Failures: 1.
Exiting with EXIT_CODE=1

    Shows the test coverage is there.

    Migration is not my best but going off @quietone in #16 mentioning this is still valid as a migration maintainer.
    Solution matches the MR

    Believe this one is good, but if wrong I apologize.

contrib.social Blog FAQ Discussions
Production build 0.71.5 2024