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

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

neclimdul’s picture

Status: Active » Needs review
FileSize
675 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

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

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

Assigned: Unassigned » rajeevk
Berdir’s picture

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

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

Status: Needs work » Needs review
FileSize
904 bytes
904 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

Status: Needs review » Reviewed & tested by the community

OK, that's fine by me.

alexpott’s picture

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.