In Drupal, especially the migrate_drupal module, we often have lines like this, which will break when moving to PSR-4 in #2247991: [May 27] Move all module code from …/lib/Drupal/… to …/src/… for PSR-4.

      drupal_get_path('module', 'migrate_drupal') . '/lib/Drupal/migrate_drupal/Tests/Dump/Drupal6VocabularyField.php',

To avoid this kind of trouble (and improve performance as a side effect), we can do this instead, which works both for PSR-0 and PSR-4, if the calling code is in the same subdirectory structure.

      dirname(__DIR__) . '/Dump/Drupal6VocabularyField.php',

Some of these were cleaned up in #2083547: PSR-4: Putting it all together, but new ones were introduced in #2121299: Migrate in Core: Drupal 6 to Drupal 8. Maybe the future will introduce even more.

We should reduce the amount of babysitting needed for the PSR-4 transition as much as possible, so let's do this now instead of waiting for the final migration. In an ideal world, #2247991: [May 27] Move all module code from …/lib/Drupal/… to …/src/… for PSR-4 will be nothing more but running the switch-psr4.sh script.

CommentFileSizeAuthor
#16 interdiff.txt1.5 KBjessebeach
#16 ps4r-dir-method-2248149-16.patch73.58 KBjessebeach
#14 ps4r-dir-method-2248149-12.patch73.55 KBjessebeach
#14 interdiff.txt6.4 KBjessebeach
#11 interdiff.txt61.82 KBjessebeach
#11 ps4r-dir-method-2248149-11.patch73.81 KBjessebeach
#1 2248149.1.patch56.67 KBdonquixote
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

donquixote’s picture

donquixote CreditAttribution: donquixote commented
Status: Active » Needs review
FileSize
2248149.1.patch56.67 KB
hass’s picture

hass CreditAttribution: hass commented

Aehm... Some years ago we have been told to use the drupal api and not php dirname with "__DIR__". Was this because of php compatibility?

donquixote’s picture

donquixote CreditAttribution: donquixote commented

The original and legitimate reason is if you want to reference files in another module, so relative paths won't be reliable. This is because modules can exist in various places such as modules, core/modules, sites/all/modules, sites/all/modules/contrib, etc.

However, since then this "best practice" has been blindly spread and promoted all over the place, without actually thinking about why we do this. So, now people think this is magical, and use it everywhere for no reason.

hass’s picture

hass CreditAttribution: hass commented

Ok, that makes sense to me :-)

xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm commented
Priority: Normal » Major

Yeah, referencing class filepaths like that is at least icky, if not a bug. Not sure about __DIR__ though; isn't there an API function that would give us the class filepath?

sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented

There is no API function for this.

However, given how much code we need to update here, it would make a ton of sense to add a helper method to the Migrate test base class à la $this->getDumpDirectory() instead of swapping out one hard-coded location with another.

→ If we move the files once again in the future, we just need to change 1 line, instead of 100+ lines.

donquixote’s picture

donquixote CreditAttribution: donquixote commented

If you look at MigrateTestBase::prepare() and MigrateTestBase::loadDumps(), where these file paths are used:

In most cases, this just extracts the class name + namespace from the file name + file contents, which seems pretty stupid to me. Instead we could simply pass the class name with namespace directly.

And this line in loadDumps() also looks weird:

      (new $class(Database::getConnection('default', 'migrate')))->$method();

We do this without even checking whether the method exists. And most of the time, the method is just 'load'.
And 'load()' is a misleading name for a method that is mostly about creating db tables.

Imo this entire system is weird. We should go for a simple solution here, just use __DIR__, and then find a way to really clean this up.

donquixote’s picture

donquixote CreditAttribution: donquixote commented

In general, all this passing around of strings (class names, paths and method names) is inferior to directly instantiating the class and calling the method, IMO.

Directly instantiating and calling can give us IDE inspections with "find usages" and type compatibility checks. Passing around strings accomplishes none of this.

EDIT: I did not study the migrate module a lot, so these are just quick thoughts from the top of my head.

donquixote’s picture

donquixote CreditAttribution: donquixote commented

Btw, the load() has no super method or interface declaration anywhere. Although it definitely should!

  /**
   * {@inheritdoc}
   */
  public function load() {
xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm commented
Priority: Major » Critical
Issue tags: +beta blocker

Actually, this is a beta blocker, because it blocks #2247991: [May 27] Move all module code from …/lib/Drupal/… to …/src/… for PSR-4.

jessebeach’s picture

jessebeach CreditAttribution: jessebeach commented
FileSize
ps4r-dir-method-2248149-11.patch73.81 KB
interdiff.txt61.82 KB

However, given how much code we need to update here, it would make a ton of sense to add a helper method to the Migrate test base class à la $this->getDumpDirectory() instead of swapping out one hard-coded location with another.

Here's the method that sun suggests in #6.

Status: Needs review » Needs work

The last submitted patch, 11: ps4r-dir-method-2248149-11.patch, failed testing.

sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented 
+++ b/core/modules/migrate/lib/Drupal/migrate/Tests/MigrateTestBase.php
@@ -79,6 +79,16 @@ protected function prepare(MigrationInterface $migration, array $files = array()
+    return dirname(__DIR__) . '/Dump';

__DIR__ without dirname() yields the right path within the /Tests directory.

jessebeach’s picture

jessebeach CreditAttribution: jessebeach commented
Status: Needs work » Needs review
FileSize
interdiff.txt6.4 KB
ps4r-dir-method-2248149-12.patch73.55 KB 
+++ b/core/modules/migrate_drupal/lib/Drupal/migrate_drupal/Tests/d6/MigrateDrupal6Test.php
@@ -79,66 +79,66 @@ public function tearDown() {
    */
   public function testDrupal6() {
-    $path = drupal_get_path('module', 'migrate_drupal');
+    $tests_path = dirname(__DIR__);
     $dumps = array(
-      $path . '/lib/Drupal/migrate_drupal/Tests/Dump/Drupal6ActionSettings.php',
-      $path . '/lib/Drupal/migrate_drupal/Tests/Dump/Drupal6AggregatorFeed.php',
-      $path . '/lib/Drupal/migrate_drupal/Tests/Dump/Drupal6AggregatorItem.php',
-      $path . '/lib/Drupal/migrate_drupal/Tests/Dump/Drupal6AggregatorSettings.php',
-      $path . '/lib/Drupal/migrate_drupal/Tests/Dump/Drupal6Block.php',

Oops, missed this. Rerolling.

Let's keep this issue extremely narrowly focused. It's a beta blocker. If you find code that could do with refactoring, please open another issue.

As far as I can tell, this patch resolves the original issue.

Status: Needs review » Needs work

The last submitted patch, 14: ps4r-dir-method-2248149-12.patch, failed testing.

jessebeach’s picture

jessebeach CreditAttribution: jessebeach commented
Status: Needs work » Needs review
FileSize
ps4r-dir-method-2248149-16.patch73.58 KB
interdiff.txt1.5 KB

Just saw sun's comment in #13. Rerolled.

Also, I had to put the getDumpDirectory in MigrateDrupalTestBase, not MigrateTestBase. MigrateDrupalTestBase is in the migrate_drupal module which has the Dump directory, whereas MigrateTestBase is in the migrate module.

benjy’s picture

benjy CreditAttribution: benjy commented

This looks like a good clean up to me.

I just wrote some migrations from WordPress the other day and I still used dump files for testing so could we move getDumpDirectory()into MigrateTestBase instead?

jessebeach’s picture

jessebeach CreditAttribution: jessebeach commented

I just wrote some migrations from WordPress the other day and I still used dump files for testing so could we move getDumpDirectory()into MigrateTestBase instead?

MigrateTestBase is in the migrate module but the Dump directory is in the migrate_drupal module, where MigrateDrupalTestBose is located. So in order to move the method getDumpDirectory into the MigrateTestBase class, we'd have to be really specific about the location of the Dump directory. Maybe just use drupal_get_path to the migrate module instead of __dir__?

donquixote’s picture

donquixote CreditAttribution: donquixote commented

I don't care so much for whether we use dirname(__DIR__) or getDumpDirectory().. The latter is probably preferable, since you don't need to be super careful about which directory you are currently in.

But I think the superior solution would be to actually pass around objects instead of file paths.

  function setUp() {
    ..
    $dumps = array(
      new ADump,
      new BDump,
      new CDump,
    );
    ...
  }

So, what about the database connection? We simply remove that from the constructor signature, and put it in the load() method instead.

interface DumpInterface {
  function load($dbConnection);
}
sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented
Status: Needs review » Reviewed & tested by the community

Adding the method to the MigrateDrupalTestBase class is fine + correct. It is only needed for the tests that extend from the class.

Let's not prematurely optimize this. Other test base classes for different migrations can simply copy those 3 lines.

donquixote’s picture

donquixote CreditAttribution: donquixote commented

I am fine if this is committed as proposed in #16.

I would still be interested in some feedback on #19. If you agree this is a reasonable direction, I will post a follow-up and produce a patch.

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
CreditAttribution: webchick commented
Status: Reviewed & tested by the community » Fixed

I don't know about #19 enough to really comment, but happy to knock off a beta blocker. :)

Committed and pushed to 8.x. Thanks!

  • Commit b803cd3 on 8.x by webchick: 
    Issue #2248149 by jessebeach, donquixote, sun: Use __DIR__ instead of...
donquixote’s picture

donquixote CreditAttribution: donquixote commented
Related issues: +#2254157: Refactor Dump classes in migrate_drupal

Follow-up: #2254157: Refactor Dump classes in migrate_drupal

Status: Fixed » Closed (fixed)

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