Same named directories in file path cause issue on re-install

[CNanninga]

I have the following situation: I've already run my install the first time, meaning all files from Magento modules have been copied into the codebase (copy, not symlink) and the composer.lock file is already present. On a new clone of this codebase where the vendor directory doesn't exist, I run "composer install" to pull down the components into vendor. This results in the modman-based copy operation being performed again, but a problem happens only in the specific case where the same directory name occurs twice in a row in one of the file paths. Example:

One of my dependency repos has the following line in modman: app/code/community/CLS/Widgets app/code/community/CLS/Widgets

One of the files there is this one, which has already been copied into the codebase on the first install: htdocs/app/code/community/CLS/Widgets/Model/Widget/Widget/Instance.php

Notice "Widget/Widget" in the file path. This copy operation worked fine on the first install, but when running "composer install" again with the files already in place, I end up with the following file in my codebase: htdocs/app/code/community/CLS/Widgets/Model/Widget/Instance.php

i.e., the extra "Widget" in the file path was somehow removed in the copy destination, and the file was copied into the wrong place.

[Flyingmana]

could you please point to a public available module and a workflow how to reproduce the problem?

[CNanninga]

Sure. First, take a look at this repo:

https://bitbucket.org/cnanninga/magento-example

You'll notice that the only model file is located at app/code/local/Nanninga/Test/Model/Widget/Widget/Instance.php

You can test the bug by cloning this repo: https://bitbucket.org/cnanninga/composer-test

. . . which contains the module files already copied into a Magento codebase. Once you've cloned this, run "composer install" from the root. A git status should show immediately that there is a new file at htdocs/app/code/local/Nanninga/Test/Model/Widget/Instance.php, which is a result of the Magento Composer Installer copying Widget/Widget/Instance.php here instead of the right location.

[davidalger]

Here the test case I'm using to debug this. The call to git status should result in nothing, but shows a new file, which is not present in the test magento-module repository. The fix in commit c812c5c actually introduces a different sort of replication bug when the modman paths differ, so I've got to chase that one down now… unless someone else has a solution. ;)

dalger:04:45 PM:/server/proj$ git clone https://github.com/davidalger/composer-basename-bug-mage.git
Cloning into 'composer-basename-bug-mage'...
remote: Counting objects: 16161, done.
remote: Compressing objects: 100% (7519/7519), done.
remote: Total 16161 (delta 6641), reused 16161 (delta 6641)
Receiving objects: 100% (16161/16161), 19.87 MiB | 3.63 MiB/s, done.
Resolving deltas: 100% (6641/6641), done.
Checking out files: 100% (12063/12063), done.
dalger:04:49 PM:/server/proj$ cd composer-basename-bug-mage/
dalger:04:50 PM:/server/proj/composer-basename-bug-mage$ composer install
Loading composer repositories with package information
Installing dependencies (including require-dev) from lock file
  - Installing magento-hackathon/magento-composer-installer (dev-master 2.0.0-beta1)
    Cloning 2.0.0-beta1

  - Installing davidalger/composer-basename-bug-case (dev-master fc5b0d6)
    Cloning fc5b0d6d30523523885632101bb055de670c2ec7

magento-hackathon/magento-composer-installer suggests installing magento-hackathon/composer-command-integrator (*)
Generating autoload files
dalger:04:50 PM:/server/proj/composer-basename-bug-mage$ git status
# On branch master
# Untracked files:
#   (use "git add <file>..." to include in what will be committed)
#
#   htdocs/app/Foobar/Widget.php
nothing added to commit but untracked files present (use "git add" to track)
dalger:04:50 PM:/server/proj/composer-basename-bug-mage$ ls -1R htdocs/app/Foobar/
Foobar
Widget.php

htdocs/app/Foobar//Foobar:
Widget.php
dalger:04:50 PM:/server/proj/composer-basename-bug-mage$ 

[Flyingmana]

will not get fixed, as it is in original modman only possible with --force and would make problems there, too. (as it simple overwrites) So $project should fix their modman file instead.

https://twitter.com/kalenjordan/status/449722175851753472

[davidalger]

@Flyingmana I completely disagree with what you're saying, and for the record, that is completely unrelated to the issue at hand. This isn't an issue with linking failing because something is already there, the bug is that the copy (not link, it doesn't affect the link method) is putting the files in the wrong place.

If you look at commit 7a9ff2d on my fork, you'll see that I have implemented a unit test which demonstrates the bug. Take a look if you will, but this is absolutely not due to having a broken modman file.

Are you trying to say that you not accept a pull-request which resolves this bug?

[Flyingmana]

linux cp behavior for cp -R some/dir/path some/other/path: results in some/other/path/path

what I say is, that I dont accept the initial reported issue as a bug. This says nothing about my acceptance of pull requests. But I probably will compare the changes with the original modman behavior.

[colinmollenhour]

This is why I don't like the --copy option, there are too many conflicts since you can't easily tell the difference between a file that is safe to delete and one that is not (whereas symlinks are pretty much always safe to delete). So for the people that want to use --copy, I think the only way is to essentially assume --force. That is if some/other/path already exists before the copy, it should be removed. Dangerous, yes, which brings me back to just sticking with symlinks.

[kalenjordan]

If I'm following this correctly, the root cause of this issue is:

linux cp behavior for cp -R some/dir/path some/other/path: results in some/other/path/path

So it's not technically speaking a bug in modman or the composer module, it's a limitation of the implementation of cp. And @davidalger implemented a workaround for this in the composer copy deploy strategy in c812c5c (with bonus points for including a test case to demonstrate the fix)

But @Flyingmana you don't want to merge that fix?

I haven't reviewed the code in detail or tested it, but if the patch resolves the issue, I can't see why you wouldn't want to merge it?

[Flyingmana]

please comment the pull request related comments on pull request :)

[kalenjordan]

Oops sorry I was linked directly here from twitter - I can see on the PR that tests are failing and that seems to be the only thing holding it up!

Thanks Daniel!

[davidalger]

@kalenjordan Yeah… still working on figuring out an actual solution. Forgot to run unit tests on my first pass. :/ I've got 2 failing tests still.

@Flyingmana Ok. I think you're misreading the issue then. :) The root cause is not due to behavior of linux cp. I understand how that works, this is different. And I'll also note that implementing a test to reproduce the bug requires the use of the force flag too.

I've created a 2nd branch on my fork "issue-76-tests" which has the unit test isolated from my modifications in order to demonstrate the bug, then I added calls to tree so it will print the output for debugging purposes.

Here is the output from the test:

dalger:04:35 PM:/server/proj/magento-composer-installer$ phpunit --filter testCopyDirToDirOfSameName
PHPUnit 4.1-dev by Sebastian Bergmann.

Configuration read from /Volumes/Server/proj/magento-composer-installer/phpunit.xml.dist

F

 -- 1st pass tree
/usr/local/zend/tmp/testCopyDirToDirOfSameName/magento_dir/dest/root
└── subdir
    └── subdir
        └── test.xml

2 directories, 1 file

 -- 2nd pass tree
/usr/local/zend/tmp/testCopyDirToDirOfSameName/magento_dir/dest/root
└── subdir
    ├── subdir
    │   └── test.xml
    └── test.xml

2 directories, 2 files

Time: 135 ms, Memory: 5.75Mb

There was 1 failure:

1) MagentoHackathon\Composer\Magento\Deploystrategy\CopyTest::testCopyDirToDirOfSameName
Failed asserting that file "/usr/local/zend/tmp/testCopyDirToDirOfSameName/magento_dir/dest/root/subdir/test.xml" does not exist.

/Volumes/Server/proj/magento-composer-installer/tests/MagentoHackathon/Composer/Magento/Deploystrategy/CopyTest.php:54
phar:///usr/local/Cellar/phpunit/3.7.28/libexec/phpunit-3.7.28.phar/phpunit/TextUI/Command.php:176
phar:///usr/local/Cellar/phpunit/3.7.28/libexec/phpunit-3.7.28.phar/phpunit/TextUI/Command.php:129

FAILURES!                            
Tests: 1, Assertions: 2, Failures: 1.
dalger:04:35 PM:/server/proj/magento-composer-installer$ 

Note that in the 2nd tree which is printed, the file test.xml shows up twice. This should not be the case. In summary, the nodes in the path with the same name are causing trouble walking the path because basename is being used to identify whether or not it must descend or iterate contents. On the first pass everything is fine because nothing exists and it simply does a recursive copy vs descending into the directory tree and copying everything individually.

[davidalger]

@colinmollenhour - Totally agree. Love links and prefer them here too! But have to use copy in this case to build something we can wrap up in a tgz. It's complicated why we need the copy. Since most don't use it, I'm guessing it's less tested than the others too.

[Flyingmana]

ok, you are right, I really misunderstood the problem.

It seemed to absurd actually having a naming of Model_WidgetWidget...

[Flyingmana]

lets see, so you have a modman entry of root dest/root and in root you have subdir/subdir/file?

does this test package match your case? https://github.com/magento-hackathon/magento-composer-installer/tree/93572e192357a2a961afe9eac1f9dc019057356a/tests/res/packages/issue76

[davidalger]

Looked it over and I believe that matches the case, yes. I've pushed up more changes to the PR for this which I believe resolve the issue, and which pass on all the unit tests for 2.0.0-beta1.

[Flyingmana]

ok, on the first execution it generates:

/tests/FullStackTest/htdocs/app/design/frontend/test$ tree
.
└── default
    └── issue76
        └── subdir
            └── subdir
                └── issue76.phtml

on the second run we have:

/tests/FullStackTest/htdocs/app/design/frontend/test$ tree
.
└── default
    └── issue76
        ├── design
        │   └── subdir
        │       └── subdir
        │           └── issue76.phtml
        └── subdir
            └── subdir
                └── issue76.phtml

So I now add a NotExists test for app/design/frontend/test/default/issue76/design/subdir/subdir/issue76.phtml

Something clearly is wrong here. But I need real existing modules to verify the behavior and expectations.

[davidalger]

With that directory output, it's not a correct test. The first execution is fine. With this bug, the second output would look like this:

/tests/FullStackTest/htdocs/app/design/frontend/test$ tree
.
└── default
    └── issue76
        └── design
            └── subdir
                ├── subdir
                │   └── issue76.phtml
                └── issue76.phtml

The NotExists check should be for the file which shows up in the 2nd round. I.e. for app/design/frontend/test/default/issue76/design/subdir/issue76.phtml

You can reference the test I put together which reproduced the bug and correctly failed prior to the work I've done on this branch testCopyDirToDirOfSameName if you'd like.

I'll have to see if I can dig up a public facing module which exhibits this behavior and post a link. I do have this test case one which reproduces the issue: https://github.com/davidalger/composer-basename-bug-case.git

[davidalger]

Here is the best way to mock the issue outside the confines of a unit test:

dalger:12:03 PM:/server/proj/magento-composer-installer$ mkdir mage
dalger:12:03 PM:/server/proj/magento-composer-installer$ tar -xzf ~/Dropbox/CLS_Dev/releases/community-1.8.0.0.tgz --strip-components 1 -C mage/
dalger:12:03 PM:/server/proj/magento-composer-installer$ cd mage/
dalger:12:03 PM:/server/proj/magento-composer-installer/mage$ mate composer.json
dalger:12:03 PM:/server/proj/magento-composer-installer/mage$ cat composer.json 
{
    "minimum-stability": "dev",
    "repositories": [
        {
            "type": "composer",
            "url": "http://packages.firegento.com"
        },
        {
            "type": "vcs",
            "url": "https://github.com/davidalger/composer-basename-bug-case.git"
        }
    ],
    "require": {
        "magento-hackathon/magento-composer-installer":"dev-master",
        "davidalger/composer-basename-bug-case":"dev-master"
    },
    "extra": {
        "magento-root-dir": "./",
        "magento-force": true,
        "magento-deploystrategy": "copy"
    }
}
dalger:12:03 PM:/server/proj/magento-composer-installer/mage$ composer install
Loading composer repositories with package information
Installing dependencies (including require-dev)                        
  - Installing magento-hackathon/magento-composer-installer (dev-master 2.0.0-beta1)
    Cloning 2.0.0-beta1

  - Installing davidalger/composer-basename-bug-case (dev-master 474cd16)
    Cloning 474cd16bc44848f9f8b3b2caf14ad2037bac3353

magento-hackathon/magento-composer-installer suggests installing magento-hackathon/composer-command-integrator (*)
Writing lock file
Generating autoload files
dalger:12:04 PM:/server/proj/magento-composer-installer/mage$ tree app/Foobar/ app/Bog/
app/Foobar/
└── Foobar
    └── Widget.php
app/Bog/
└── Bog.php

1 directory, 2 files
dalger:12:04 PM:/server/proj/magento-composer-installer/mage$ rm -rf vendor/davidalger/
dalger:12:04 PM:/server/proj/magento-composer-installer/mage$ composer install
Loading composer repositories with package information
Installing dependencies (including require-dev) from lock file
  - Installing davidalger/composer-basename-bug-case (dev-master 474cd16)
    Cloning 474cd16bc44848f9f8b3b2caf14ad2037bac3353

Generating autoload files
dalger:12:04 PM:/server/proj/magento-composer-installer/mage$ tree app/Foobar/ app/Bog/
app/Foobar/
├── Foobar
│   └── Widget.php
└── Widget.php
app/Bog/
└── Bog.php

1 directory, 3 files
dalger:12:05 PM:/server/proj/magento-composer-installer/mage$ 

[bragento]

Unfortunately this change seems to have some negative side effects. Since 2.0.0-beta2 we are experiencing the following behaviour:

1. Magento root dir is not empty. The app folder already exists
2. We try to install the project with the copy deploy strategy
3. Instead of copying the contents of vendor/myVendor/myModule/app/* into the magentoRoot/app/ directory, it places the app folder itself within the magentoRoot/app/ directory

[Flyingmana]

hi @bragento I tried to reproduce this in a testcase I added https://github.com/magento-hackathon/magento-composer-installer/commit/2eef3df4e6ff77642cff4d24df4b4bea58944c74 I think this represents what you are reporting, but I cant find the result you are describing. Is it possible you contribute a testcase which reproduces your problem? Feel free to open a new Issue for your problem or contact me directly

[davidalger]

Ported from the comment made on a source-line: https://github.com/magento-hackathon/magento-composer-installer/commit/c812c5cf25eaea201abb485fb61b5edacefdf54c#commitcomment-6045581 — Makes most sense to continue discussion here and keep it in one place. :)

davidalger added a note a day ago @bragento Could you post the exact contents of your modman file declaring the copy? I’m gong to try and reproduce this, add a test for it, then see what can be done to solve the regression.

bragento added a note a day ago @davidalger Thanks for your quick reply. Actually the problem can be reproduced pretty easily. Modman of Magento Core:

• / .htaccess .htaccess

Now simply create the Magento-Root Dir. Add an empty "app" folder to it. Trigger composer install. The Magento app folder itself ends up within the created empty app folder instead of its content

[Flyingmana]

@davidalger I added a testcase for this. Any Ideas how to solve this, as it seems Issued by your patch?

Also, there seems to be a fullstack testcase missing for your changes. If I replace /src/MagentoHackathon/Composer/Magento/Deploystrategy/Copy.php:53 with if (basename($sourcePath) === basename($destPath)) { nothing of the fullstack cases breaks, only a single unittest. Could mean, the unittest itself is wrong?

[davidalger]

Not sure yet since I haven't yet had a chance to attempt reproducing it, but I will be doing that shortly. I need to get the full-stack tests working on my machine though… still getting this on each of the three modes when I run the full-stack tests:

Failed asserting that file "/Volumes/Server/proj/magento-composer-installer/tests/FullStackTest/artifact/magento-hackathon-magento-composer-installer-999.0.0.zip" exists.

Other artifacts are being created though:

dalger:02:39 PM:/server/proj/magento-composer-installer$ ls -l tests/FullStackTest/artifact/
total 168
-rw-r--r--  1 dalger  admin      0 Apr 18 14:36 empty
-rw-r--r--  1 dalger  admin  70812 Apr 18 14:38 magento-hackathon-magento-composer-installer-2.0.0.0-beta2.zip
-rw-r--r--  1 dalger  admin    700 Apr 18 14:38 magento-hackathon-magento-composer-installer-test-issue-87-1.0.0.zip
-rw-r--r--  1 dalger  admin    861 Apr 18 14:38 magento-hackathon-magento-composer-installer-test-updatefileremove-1.0.0.zip
-rw-r--r--  1 dalger  admin    686 Apr 18 14:38 magento-hackathon-magento-composer-installer-test-updatefileremove-2.0.0.zip
dalger:02:39 PM:/server/proj/magento-composer-installer$ 

The full-stack test would appear to be incorrect; the failing individual unit-test is the one I added (testCopyDirToDirOfSameName) to regression test the bug I was fixing. So I'm going to figure out what's up with the full-stack test for this and make sure it works correctly as well as create a test for the regression.

Any ideas why the full-stack tests wouldn't be failing? I can run them in a CentOS VM just fine…but on my primary OS X setup not so much.

[davidalger]

Alright…since I got the full-stack tests running per the changes I made in pull request #90, I might actually get somewhere on this now. ;)

[Flyingmana]

I maybe fixed the problem, at least I could create a unittest for it, a fullstack test, and nothing fails now. If the problem still exists, we need to try again with creating another unittest.

[davidalger]

Looks like a valid fix. @bragento, could you test this against your scenario and report back?

[bragento]

Thanks a lot for your support so far. Unfortunately I'm still having some trouble getting everything to work as it did/does on 1.x I may be on the wrong branch... dev-master c18c489: Same, known, error ==> app folder is deployed within app folder

2.0.0-beta3: Syntax error ==> PHP Parse error: syntax error, unexpected '[' in /home/brandung/www/test_beta3/vendor/magento-hackathon/magento-composer-installer/src/MagentoHackathon/Composer/Magento/Plugin.php on line 37

dev-plugin-rewrite 6618b9c: Fatal Error ==> PHP Fatal error: Call to a member function addPackage() on a non-object in /home/brandung/www/test_1x/vendor/magento-hackathon/magento-composer-installer/src/MagentoHackathon/Composer/Magento/Installer.php on line 267

[Flyingmana]

the dev-plugin-rewrite branch is the correct one.

I assume the error happens during the use of the composer command integrator? I have to admit, I did not test/use this in a while. So I have to ask, does it work before using the command Integrator?

I dont have a working test solution for the command integrator yet, and looking at the source I think there could be some problematic parts. At least I tried to patch it, and maybe it does not throw an error now, but if there is still a problem:

One way would be, you try yourself to change \MagentoHackathon\Composer\Magento\Command\DeployCommand till it executes all new actions of the installer you need. Or you have a few weeks time, so I refactor half the logic again, to make it easier to reuse from the command class.

Does this sound ok for you? And could you please test again the dev-plugin-rewrite branch?

[bragento]

Actually I just wanted to execute a regular "composer install" when I got the error. Currently we have no real use for the command line integrator since our processes are either triggered by events or our installer during the regular install/update actions. As far as I remember the error occurred during the installation of the hackathon mage composer installer. Before any custom modules of ours ever had the chance to get installed. I'll retest it later on and report back with the precise circumstances if the error is still there and if you weren't able to reproduce the error yourself till then.

Thanks a lot so far!

[davidalger]

@bragento Could you post a composer.json which reproduces the issue? I am not able to get those errors to trigger.

[raphaelpetrini]

Hi @Flyingmana I think I'm facing the same or similar issue now.

Here are the exact details on the issue:

First I have my composer.json file under my magento root directory:

{
  "require": {
    "magento-hackathon/magento-composer-installer":"*",
    "factoryx-developers/factoryx_contests":"dev-master",
    "factoryx-developers/factoryx_birthdaygift":"dev-master"
  },
  "repositories": [
    {
      "type": "vcs",
      "url": "https://github.com/magento-hackathon/magento-composer-installer"
    },
    {
      "type": "vcs",
      "url": "https://**********@bitbucket.org/**********/factoryx_contests.git"
    },
    {
      "type": "vcs",
      "url": "https://**********@bitbucket.org/**********/factoryx_birthdaygift.git"
    }
  ],
  "extra": {
    "magento-root-dir": "./",
    "magento-deploystrategy": "copy"
  }
}

And here is the content of my module composer.json files (simplified and both are the same):

{
  "name": "**********/factoryx_contests",
  "type": "magento-module",
  "license":"OSL-3.0",
  "description":"Contests module",
  "authors":[
    {
      "name":"Raphael Petrini",
      "email":"raphael@**********"
    }
  ],
  "extra": {
    "map": [
      ["locale/*", "app/locale/en_AU"]
    ]
  }
}

Under my locale folder I have:

• template/email/factoryx/contests/contests.html for one module
• template/email/factoryx/birthdaygift/customer_birthday.html for the other module

When I run composer update, everything goes fine (no error) but I end up with the following folders:

• app/locale/en_AU/template/email/factoryx/contests/contests.html (which is what I want)
• app/locale/en_AU/template/template/email/factoryx/birthdaygift/customer_birthday.html (notice the extra template folder).

If I add a third module which another locale config, it'll add an third template subfolder in the previously created ones and thus, that totally breaks my module configuration.

[Flyingmana]

does this still happen with the most recent 3.x version?

[Bravehartk2]

Any updates on that issue?