Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Avoid duplicate dependencies #14

Closed
franzliedke opened this issue Mar 26, 2015 · 19 comments
Closed

Avoid duplicate dependencies #14

franzliedke opened this issue Mar 26, 2015 · 19 comments

Comments

@franzliedke
Copy link
Owner

Studio currently suffers from the same problem that Workbench had: libraries in development with some of the same dependencies of the main project can cause conflicts with autoloading.

This needs to be solved by messing around with the Composer-generated files.

Here's my idea:

  • Composer currently generates (I think) three different files based on the different autoloading types (PSR-4 etc.). These can be merged easily.
  • After creating the autoloaders for the main project as well as the managed libraries, the type-specific files from the managed libraries should be merged into the respective ones from the main project. This should come down to merging a bunch of arrays.
  • For conflicts, the ones from the main project should be preferred.

/cc @kirkbushell

@kirkbushell
Copy link

@franzliedke hmmm, I like where you're going with this...

Personally I just don't work in a composer directory, or via workbench. I have a few cli tools that simply synchronize or update where necessary.

I honestly think you may be wasting your time here, but I'm happy to be proven wrong :)

@franzliedke
Copy link
Owner Author

Made some progress on this today. Coming this week. =)

franzliedke added a commit that referenced this issue Apr 1, 2015
This is still work in progress, as the "autoload_files.php" is not
handled yet. That one has to be handled differently, because its
array is indexed with numeric keys.

Related to #14.
franzliedke added a commit that referenced this issue Apr 1, 2015
Related to #14.
franzliedke added a commit that referenced this issue Apr 1, 2015
@RemiCollin
Copy link
Contributor

What would be the prefered behaviour when the package you're working on is a part of the main project dependency ?
I often work on package while developping application, which mean these packages are listed as dependencies in the composer.json. Right now my workaround is to maintain 2 composer.json files, one for the dev version and one for the production app.
Is it possible for studio to preffer the managed package over main project dependencies ?

@franzliedke
Copy link
Owner Author

While you work on a dependency using Studio, you should not have it in your dependency list in composer.json.

@janhartigan
Copy link

So a way around this weirdness is to do this with your composer:

composer config -g prepend-autoloader false

Or alternatively in your local composer json:

composer config prepend-autoloader false

This likely causes issues with other composer plugins because it changes the order of the autoloader stack. However, in my case I don't use any others and that then makes this package work like Workbench. I still think it's a good idea to try to make this package load its autoloader after the primary one. The issue seems to be with how it's modifying Composer's autoload file, but I'm not sure about best practices there. Composer has that return command which confuses things. This is how it normally looks after running the dumpautoload for this package:

// autoload.php @generated by Composer

require_once __DIR__ . '/composer' . '/autoload_real.php';

// @generated by Composer Studio (https://github.com/franzliedke/studio)

require_once __DIR__ . '/../../../API/repo\vendor\autoload.php';

return ComposerAutoloaderInit17d3a149ef691d117c0e0052e62c942d::getLoader();

And this is how it would ideally look if it were to work exactly like Workbench:

// autoload.php @generated by Composer

require_once __DIR__ . '/composer' . '/autoload_real.php';

$loader = ComposerAutoloaderInit17d3a149ef691d117c0e0052e62c942d::getLoader();

// @generated by Composer Studio (https://github.com/franzliedke/studio)

require_once __DIR__ . '/../../../API/repo\vendor\autoload.php';

return $loader;

Thoughts, @franzliedke?

@franzliedke
Copy link
Owner Author

@janhartigan I'm confused. The method you linked to takes care of merging the type-specific autoload files (for "psr0", "psr4" etc.) from managed packages.

But the autoload.php file itself is not even touched anymore with these changes.

So what are you suggesting? :)

@janhartigan
Copy link

@franzliedke I probably misunderstood how the autoload file is modified by this package. Is it a feature of composer's plugin architecture?

In any case, I was referring to the problem that arises with what @RemiCollin mentioned. I have 3 separate applications that have a composer dependency on one of our shared libraries. These applications all have the composer.json dependency and we can't easily get rid of that. In order to make development easier, we've so far used the Workbench to let composer prepend the shared library's autoloader. This would mean that any time one of our package's classes were loaded composer would prefer the one that was loaded second.

Because of the bit of code I wrote out above, Studio doesn't allow for this unless you tell composer to append (rather than prepend) subsequently-loaded autoloaders. It's the ::getLoader() function, after all, that initializes the spl_autoload_register() method, so with the way it currently is set up, the Studio packages' autoload runs first, and the application's autoload runs after.

@janhartigan
Copy link

And just to clarify, this previously worked under Workbench because Workbench was loaded by the Laravel application well after the autoload.php file ran the first getLoader(). This would mean that the Workbench packages' spl_autoload_register() would prepend onto the autoloading stack (as is the default behavior in composer).

@janhartigan
Copy link

It's also worth mentioning that I'm not totally against the idea of keeping it the way it is, but perhaps with a note to people about modifying the composer config to append autoloaders. One problem with prepending has always been that it totally bungles autoloaded helper function files by loading the files in the vendor directory before the ones in the workbench. Interested to hear you thoughts on this.

@Rjs37
Copy link

Rjs37 commented Sep 21, 2015

I think until a decision has been made, this behaviour (or lack thereof) should be described in the readme. This confused me quite a bit until I came across this issue and applied the suggested change from @janhartigan

If I do a composer update on the package then all hell breaks loose and I get duplicate dependencies but I'm just avoiding that and only doing the composer updates on my main project. With that it seems to work as I need it to.

EDIT: I didn't realise that the way things are being done had been changed already when I said the above.

@franzliedke
Copy link
Owner Author

This is now not a problem anymore.

Managed packages will not be hacked into the autoloader files anymore, but instead they will be symlinked into the vendor dir. Composer already provides this for free using path repositories.

Next release is coming up. :)

@janhartigan
Copy link

@franzliedke awesome, thank you very much!

@janhartigan
Copy link

@franzliedke so out of curiosity, would studio use a behind-the-scenes composer method for adding a new path repository so that we wouldn't have to change our composer.json?

@franzliedke
Copy link
Owner Author

@janhartigan Yes, indeed. Unfortunately, it only works for packages that haven't been registered with Packagist, so far... :(

I'm on it, though. Might need a new event or method in Composer to get this working.

@franzliedke
Copy link
Owner Author

I'll post here when I need your lobby for my Composer PR ;)

@janhartigan
Copy link

@franzliedke ok excellent. Let me know and I'll do what I can.

@franzliedke
Copy link
Owner Author

I'll post here when I need your lobby for my Composer PR ;)

That time has finally come. I managed to send a pull request to Composer. The change was super simple, if I had known that earlier...

Now I need your support to get it into Composer. :)

@janhartigan
Copy link

I must have enormous power, because they merged it without my even saying anything.

Great work, Franz!

@franzliedke
Copy link
Owner Author

Yay, the release is on its way. I still need to go through everything again and prepare a blog post, but it will happen this week. :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants