Make the class variables protected for Migration

Round 1

@daffie I know, thank you!
Let me continue with this work :)

Let me see...

Starting with a reroll first.

There wasn't a more direct error because the check for the property was done via a isset call, which always returned FALSE after making it protected, and hence all the errors. This one change might fix most (or all) errors. Let's see.

*cough*

I have thought we do not RTBC issues without asking the subsystem maintainer. What's the point of having a subsystem maintainer if we are making changes to the architecture he is supposed to maintain without asking him?

I'm not against making the properties protected although it does seem a bit late in the game to be making such API changes. This is also going to break the Drush runner, so it would be good if we could open a pull request for that as well.

$migration->get('property_name') is worse UX than a public property IMO. I think we should at least add the appropriate getter/setter methods to the MigrationInterface. But maybe not for all properties, since many will rarely be accessed.

I think we need methods for:

• $process - Because it's already used by the load plugin and will be a common one to change from contrib in hook_entity_alter() and such.
• $systemOfRecord - We don't use this yet, but the D7 module did, and it had a getter.
• $trackLastImported - Used by Sql, lets add a getter.
• $source and $destination are both used by Drush. Not sure if that's enough of a reason to add the methods here though since they might get confused with getSourcePlugin().

+++ b/core/modules/migrate/src/Entity/Migration.php
@@ -144,14 +144,14 @@ class Migration extends ConfigEntityBase implements MigrationInterface, Requirem
+  protected $destinationIds = FALSE;

Can we change this while we're at it to default to an empty array?

Getting started with feedback in #26 and #27. I'd love some feedback to proceed.

I have used the parent method get() or set() in the getters and setters. Do you think I should directly access the properties?

Lastly, I was also thinking of adding a parameter to the getMigrationDependencies() getter to get either required or optional or all migrations. Is this too much out of scope? I know there might be other concerns and have not implemented it in this patch but I'd like to know your thoughts on this.

For the $process variable a getter and a setter. - Yes
For the $systemOfRecord variable a getter - Both getter and setter.
For the $trackLastImported variable a getter - Getter and setter
For the $source variable you would like? - Leave for now
For the $destination variable you would like? - Leave for now

I have used the parent method get() or set() in the getters and setters. Do you think I should directly access the properties?

I think we should access the properties directly, I can't see any reason not to.

Lastly, I was also thinking of adding a parameter to the getMigrationDependencies() getter to get either required or optional or all migrations. Is this too much out of scope?

Out of scope here, open a new issue if you want to discuss that further. It seems like a reasonable proposal.

I have added the setters for trackLastImported and systemOfRecord. The getters/setters for source and destination were already implemented back in #28. I am updating the IS.

@daffie: Quite a lot of work happened between #28 and #31 and I don't see why I should go back to #20. If we definitely shouldn't have those methods, then I can edit the patch in #28 (now #31).

Besides, we are using the get() and set() method at least for source property and @benjy commented earlier that that is worse UX than directly accessing the property. Are you sure that you want to revert to the get('source') calls from getSource(), especially since it is implemented now. I know we are not changing API in beta but we are already adding a lot of methods. Does adding a few more methods matter?

And I forgot to mention migration_dependencies. The same argument as source property applies there too. Should we just change it to get('migration_dependencies') now?

@hussainweb, the reason I mentioned not adding getSource() and getDestination() is because a) they're only likely to be used by core and drush and b) I thought it might cause confusion with getSourcePlugin() and getDestinationPlugin(). Do you feel strongly about adding them?

@benjy: Not strongly, but we have a getSource() call in the patch. Are you okay with get('source')? If yes, then we can remove.

Plus, getSource definitely looks like a getter and I think because of the convention, it should not be confused with getSourcePlugin. I am not saying it won't happen, but it at least matches the convention. If we add another getter later and if it is not getSource due to this reason, it would be more confusing.

Instead, do you think we should rename one of the properties, if the naming convention could really be confusing?

I was leaning towards not adding getSource and then adding it later if we had a reason to. As you say, getSource() is already used so lets just leave the get/set/Source/Destination methods you already have in your patch.

Instead, do you think we should rename one of the properties, if the naming convention could really be confusing?

No, lets not rename anything.

I'd like to have some consensus before making the change. Also, I think getMigrationDependencies() is nice to have, even if it may not be used outside core and drush.

@daffie: I understand, but if get('migration_dependencies') is considered worse (as in #26), then you need the independent getter.

BTW, I have created a PR for drush as @benjy mentioned in #26. You can see the request at https://github.com/drush-ops/drush/pull/1076.

Lets remove get/set/Source/Destination as @daffie has said, alexpott requested not to have unused methods and i'm happy with get('source') for an internal usage. As I previously mentioned, I think these are less likely to be used and possibly confusing anyway.

I'd like to leave getMigrationDependencies(), I think that's worthwhile and we already use it.

I'm removing get/set/Source/Destination as per #43 and earlier comments.

1. +++ b/core/modules/migrate/src/Entity/Migration.php
   @@ -117,7 +117,7 @@ class Migration extends ConfigEntityBase implements MigrationInterface, Requirem
   +  protected $idMap = array();
   

   Change to square brackets.

2. +++ b/core/modules/migrate/src/Entity/Migration.php
   @@ -382,4 +384,57 @@ public function isComplete() {
   +    if ($system_of_record != self::SOURCE && $system_of_record != self::DESTINATION) {
   

   Lets use a strict check, eg ===

   Also, i'm not sure i like this limitation, seems like something easy to mess if we added new constants.

Then I think this is ready

Fixing for point 1 (and in another place in the same file) and changed to strict check.

Regarding the second point, this would throw an exception if a constant is added and used (which is unlikely in this case). Assuming we add test coverage, this error will be immediately caught.

I don't feel too strongly for this check either as it is of limited use, but it does serve as some defense. The ideal way would be to encapsulate these constants in their own class/interface and type hint the parameter. That is the proper way of course, but probably an overkill here, and unlikely to get in in this beta stage. What do you think?

Why are we using self:: and not static:: ? That's odd.

Actually, I meant to use static and got confused and used self. Changing that.

Regarding the second point, this would throw an exception if a constant is added and used (which is unlikely in this case). Assuming we add test coverage, this error will be immediately caught.

I don't think we have tests for this stuff at all, it's mainly left over from the D7 codebase. I think we should just remove the check altogether for now.

NW to remove the checks from setSystemRecord()

Removed changes as per #52 and #53.

Looks good to me.

rerolled

Fatal error: Cannot access protected property Drupal\migrate_drupal\Entity\Migration::$process in /var/lib/drupaltestbot/sites/default/files/checkout/core/modules/migrate_drupal/src/Plugin/migrate/load/LoadEntity.php on line 107

Conflicted with #2394567: File field need associated metadata during cck_field migration, should be a simple fix.

let me see

Of course!

benjy put this to RTBC already so I put it back.

If you'd ask me, I would won't fix this. Boilerplate--

rerolled

You might want to wait until #2348875: Improving our dump files gets in because that one conflicts as well and that one has priority.

Rerolling from #63. I don't have a lot of hope for this turning green but let's see.

Looks good.

@hussainweb when you retest a patch due to random failure can you please document the reason.

Ah yes, I forgot to add a comment after hitting the retest link. It appeared to be a random failure in one of the tests. It was an out of memory error.

Committed 1e98b16 and pushed to 8.0.x. Thanks!

Fixed the beta evaluation on commit. Thanks for adding it.

Wait a minute isn't migrate beta evaluation exempted?