Problem/Motivation

Lots of test fails in migration on PHP 7 due to the Null destination.

I initially thought it was a new class but I guess we just weren't using.

Proposed resolution

Rename to NullDestination, leave everything as-is (e.g. id in the annotation).

Remaining tasks

User interface changes

API changes

Class is renamed, but it's a plugin and guess mostly used for tests, so I don't think it needs to be a critical.

Data model changes

CommentFileSizeAuthor
#7 null_migrate-2531028-7-interdiff.txt904 bytesberdir
#7 null_migrate-2531028-7.patch904 bytesberdir
#1 null_migrate-2531028-1.patch675 bytesneclimdul

Comments

neclimdul’s picture

neclimdul
He/Him
commented
Status: Active » Needs review
StatusFileSize
new null_migrate-2531028-1.patch675 bytes

I forgot to file this apparently. Phenaproxima suggested DevNull and chx suggested Sarlacc.

Here is a patch that renames it to NullDestination though which is probably a safer choice. We shouldn't have to touch anything else because the plugin name is fine and will still resolve.

benjy’s picture

benjy commented

I don't mind NullDestination but it is a bit of inconsistency, it's now the only destination with the "Destination" suffix on the class name. How about a DoNotSave destination?

jibran’s picture

jibran
he/him
Primary language Urdu
Location Toronto, Canada
commented
Status: Needs review » Needs work 
+++ b/core/modules/migrate/src/Plugin/migrate/destination/NullDestination.php
@@ -16,7 +16,7 @@
+class NullDestination extends DestinationBase {

File description also needs an update.

rajeevk’s picture

rajeevk
Location London
commented
Assigned: Unassigned » rajeevk
berdir’s picture

berdir
Primary language German
Location Switzerland
commented

No strong opinion on the classname, just suggested NullDestination because it is a common pattern to name classes after what they are. In fact, our coding/naming standards would even require us to do exactly that.

benjy’s picture

benjy commented

NullDestination because it is a common pattern to name classes after what they are. In fact, our coding/naming standards would even require us to do exactly that.

Maybe we should rename the others then?

berdir’s picture

berdir
Primary language German
Location Switzerland
commented
Status: Needs work » Needs review
StatusFileSize
new null_migrate-2531028-7.patch904 bytes
new null_migrate-2531028-7-interdiff.txt904 bytes

Not sure, certainly not here I think :)

IMHO, this rename is perfectly in line with similar changes we made for PHP7, for example renaming the typed data String class to StringData. We also didn't rename all of those to ..Data.

benjy’s picture

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

OK, that's fine by me.

alexpott’s picture

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

Committed f5cdc33 and pushed to 8.0.x. Thanks!

  • alexpott committed f5cdc33 on 8.0.x 
    Issue #2531028 by Berdir, neclimdul: Null migrate destination is a...

Status: Fixed » Closed (fixed)

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