There is a lot of inconsistency in the Migrate code base between "highwater mark" and "high water mark" in the documentation, and CamelCasing of HighWater vs. Highwater in method/member variable names.

It should be two words, and camel cased HighWater.

Should be a good Novice project to search and replace:
- highwater => high water [in documentation/comment lines]
- Highwater => High water [in documentation/comment lines]
- Highwater => HighWater [in CamelCase class/method/variable names]
- highwater => highWater [in CamelCase class/method/variable names]

I think this should only be present under core/modules/migrate and core/modules/migrate_drupal.

CommentFileSizeAuthor
#3 interdiff.txt2.23 KBbenjy
#3 2320245-3.patch17.22 KBbenjy
#1 2320245-1.patch17.14 KBbenjy

Comments

benjy’s picture

benjy commented
Status: Active » Needs review
StatusFileSize
new 2320245-1.patch17.14 KB

Every instance of highwater changed into either "high water" or highWater depending on it's usage.

Couple of API changes with a constant and the key value storage name changing.

jhodgdon’s picture

jhodgdon
she/her
Primary language English
commented
Status: Needs review » Needs work

This one should really be a local variable (non camel case):

+++ b/core/modules/migrate/src/Plugin/migrate/source/SqlBase.php
  protected function runQuery() {
     $this->prepareQuery();
-    $highwaterProperty = $this->migration->get('highwaterProperty');
+    $highWaterProperty = $this->migration->get('highWaterProperty');

Also in the Source class, ::next() method:

-      elseif (!empty($highwater['field'])) {
+      elseif (!empty($high_water['field'])) {

This is suspicious, as it is the first and only mention of $highwater in this method. Should it maybe be something else here? It is never going to be not empty, because it is never being set anywhere, as far as I can tell. Looks like a bug?

Other than that, looks great! Also I committed #2316413: Clean-up docs in MigrationInterface so this needs a reroll.

benjy’s picture

benjy commented
Status: Needs work » Needs review
StatusFileSize
new 2320245-3.patch17.22 KB
new interdiff.txt2.23 KB

Fixed the first one.

Yeah the second one is a bug. This code was copied entirely from the D7 migrate module and hasn't had much attention since. I've renamed the field to reference the class property but it's pretty obvious this code path isn't tested. That should be another follow-up since this issue is mainly docs and that could turn into a significant amount of work.

xano’s picture

xano
he/they
Primary language English
Location Southampton
commented

Merriam Webster's confirms high water is the correct spelling.

jhodgdon’s picture

jhodgdon
she/her
Primary language English
commented
Status: Needs review » Reviewed & tested by the community

OK, I'll leave it to you to file an issue on the obviously not working code.

alexpott’s picture

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

Committed 72b1c2f and pushed to 8.0.x. Thanks!

diff --git a/core/modules/migrate/src/Entity/MigrationInterface.php b/core/modules/migrate/src/Entity/MigrationInterface.php
index 98120a7..4bfb89f 100644
--- a/core/modules/migrate/src/Entity/MigrationInterface.php
+++ b/core/modules/migrate/src/Entity/MigrationInterface.php
@@ -143,7 +143,7 @@ public function getIdMap();
    * imported.
    *
    * @return int
-   *   A Unix timestamp representing the highwater mark.
+   *   A Unix timestamp representing the high water mark.
    */
   public function getHighWater();

Fixed on commit

  • alexpott committed 72b1c2f on 8.0.x 
    Issue #2320245 by benjy | jhodgdon: Fixed High water should consistently...
benjy’s picture

benjy commented
Related issues: +#2321911: Unused code in Source.php

Follow-up for #2 #2321911: Unused code in Source.php

jhodgdon’s picture

jhodgdon
she/her
Primary language English
commented

Oh good, thanks!

Status: Fixed » Closed (fixed)

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