Rename Row::stub to Row::isStub

Current there are only 2 uses of '$VARIABLE->stub()' in core

$row->stub(TRUE/FALSE); is not used YET, but that functionality exists, as a setter, to update the protected variable $stub inside the instance of \Drupal\migrate\Row

Reviewers can comment on whether that future-proofing should be preserved with the setStub method provided by the patch?

Comparing with ContentEntityBase::isDefaultRevision() - shows that Row::isStub() should also have an explicit cast to bool.

-      $this->stub = $new_value;
+      $this->stub = (bool) $new_value;

1) All tests now pass locally.

2) Also updated comment to reflect newly constrained return type.

This doesn't make sense to me. I think there should be separate getter and setter.
Now we are using isStub() to also get/set? Still a naming confusion.

+++ b/core/modules/migrate/src/Row.php
@@ -307,12 +307,12 @@ public function getHash() {
-      $this->stub = $new_value;
+      $this->stub = (bool) $new_value;

wait.

... if this does not return values now, and always is true/false... then the docs need to be updated also.

  /**
   * Flags and reports this row as a stub.
   *
   * @param bool|null $new_value
   *   TRUE when the row is a stub. Omit to determine whether the row is a
   *   stub.
   *
   * @return bool
   *   The current stub value.
   */
  public function isStub($new_value = NULL) {
    if (isset($new_value)) {
      $this->stub = (bool) $new_value;
    }
    return $this->stub;
  }

why should we passing TRUE knowing it is a stub, in order to find out *if* it's a stub.

Seems like the param should not be there.

I would also suggest renaming the property to isStub, since it is not setting/storing the stub value, but just if it is a stub or not.

6: renameStub-2275377-6.patch queued for re-testing.

Extract from Drupal\aggregator\Tests\AggregatorConfigurationTest

SQLSTATE[08004] [1040] Too many connections	PDOException	df0d7c78b47ddcb219eecad49900551e4ec136191c4f4deaf888d65c9f8a12da.php	1443	service_container_prod->getDatabaseService()

Drupal\action\Tests\BulkFormTest passes locally for me...

Testbot is overworked and at a DrupalCon, so I am retesting thinking this test fail is just a bots drunk stumble...

I don't want to start a 'bikeshed' but on reflection

1) I don't think ContentEntityBase::isDefaultRevision() should be taken as an example of good design.

2) Splitting into a isStub and setStub will not break the principle of "causing least surprise"
Whereas I cannot conceive of explaining this code to a new developer without using the phrase

"Yeah well this is the legacy code we have to work around"

, and so can't justify what we have as a new patch...

3) Since having a setter was in my first patch ... I will leave it for other to change if they wish to ... for now I will just ask for a retest

The patch in #6 makes less sense than what we have now. an "isStub" method that also supports setting doesn't fit that well.

Either we add the setter back and make isStub only return the value or I prefer we just leave this as "stub" to be honest, it's only used internally to migrate from what i'm aware.

A Row is either a stub or not - it's not something that's ever going to change dynamically during a row's lifetime. So, why not set it in the constructor?

A Row is either a stub or not - it's not something that's ever going to change dynamically during a row's lifetime. So, why not set it in the constructor?

Yes, I agree with that. RTBC!

MigrationInterface::getDestinationPlugin() has an undocumented (and apparently Boolean) parameter named $stub. Should that be renamed too?

+++ b/core/modules/migrate/src/Row.php
@@ -90,13 +90,16 @@ class Row {
+   * @param bool $stub
+   *   TRUE to create the row as a stub.
...
+  public function __construct(array $values, array $source_ids, $stub = FALSE) {

I'd think seeing $stub that it was an actual stub row object. Maybe $create_stub or such would be a better name? Edit: And if the parameter for getDestinationPlugin() is actually supposed to be like this rather than an "is", then we should rename that to the same thing instead.

Moving it to the constructor makes sense to me too.

Thanks all!

Adding a beta evaluation... stub. ;)

The other $stub/stub() instances we're dealing with directly represent the state of the Row object, this is used to validate that the destination plugin supports stubbing before creating the stub Row. We could call it... $stub_being_requested? That does make the line where it's used pretty clearly self-documenting...

Sounds OK to me.

That works, thanks!

As a Migrate improvement, this issue is a prioritized change during the beta phase per https://www.drupal.org/core/beta-changes, and I agree that the minor BC break is merited by the improved code legibility. Committed and pushed to 8.0.x (with credit for reviewers).

Hm, it occurs to me that this should probably have a small change record for the method rename.

We've never created change records before for Migrate, maybe because it's currently an un-supported/released API?

Happy to do it though if you think we should start?

@benjy, interesting point. I was thinking that it would help contrib modules writing migrations to notify them about the BC break, but maybe we don't need it for the same reasons we only did major 8.x to 8.x change records before the beta. Since this is a small break I guess there's no need to bother.

When do you think it would make sense to start adding them? For major API changes now, and then for any BC break starting during RC?

Yeah I think major API changes would warrant a CR at this point and RC seems like a good milestone to start CR's for everything else.