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

#52 Fixing the lock file #73

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

SanderSander
Copy link

@SanderSander SanderSander commented Mar 22, 2017

This fixes the composer.lock file issue #52

The only problem is when the remote repository is updated or when you switch from version, composer prompts that it can't find the .git directory of the package.

Creating a copy of the original downloaded package isn't really an option in my opinion because then it should be stored in the /vendor directory.

The initial lookup for newer versions is cached by composer and works as it should.

Also it prevents users to have the dev-master tag in the composer.json.
The symlinked directory can be checked out on any branch you would like too and is completely detached from composer.

screenshot from 2017-03-22 22-52-08

@SanderSander
Copy link
Author

SanderSander commented Mar 23, 2017

I have another commit that creates an .studio directory in the root of the project and copies the original packages to that directory.
It solves the issue with the error message from composer.

The same directory is also used to store a copy of studio.json file to detect packages that are removed, so that we can restore the original into the vendor directory.

@franzliedke
Copy link
Owner

Thanks for the initiative, I hope to review it early next week.

Copy link
Author

@SanderSander SanderSander left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It still needs some more work and I forgot a few checks, I have time this weekend to fix a few things and make the code a bit nicer. Maybe I can add a few tests also would be really nice.


}

copy(
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ugly, also missing some checks if the studio.json file exists

private function getPreviouslyManagedPaths()
{
$targetDir = realpath($this->composer->getPackage()->getTargetDir()) . DIRECTORY_SEPARATOR . '.studio';
$config = Config::make("{$targetDir}/studio.json");
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use directory seperator

{
$repoManager = $this->composer->getRepositoryManager();
$composerConfig = $this->composer->getConfig();
$targetDir = realpath($this->composer->getPackage()->getTargetDir()) . '/.studio';
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

DIRECTORY_SEPARATOR


// Creates the symlink to the package
$filesystem = new Filesystem();
if (!$filesystem->isSymlinkedDirectory($destination)) {
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure if this works on windows needs some testing

Additional check for studio.json
@jonnywilliamson
Copy link

@SanderSander Wonderful stuff! Watching this to see how it all pans out! Thanks for looking into this issue, much appreciated!

 - Fix custom installer paths
@SanderSander
Copy link
Author

SanderSander commented May 2, 2017

I made everything better testable (first time with phpspec so suggestion are welcome).

There are a few points of attention left.

  • Currently the Filesystem that is used from Composer isn't sufficient. copy, file_exists and is_dir methods are used which make testing a bit quirky. The Filesystem from composer is mostly useful for Windows and the windows junctions, mainly because I'm not that familiar with developing on Windows.
  • The Config class is only able to load the config directly from the file, should be nice to have the ability to load json for testing.

@SanderSander
Copy link
Author

@franzliedke Any change to give some feedback about the current approach?

@franzliedke
Copy link
Owner

Sorry for the delay. And thanks for your hard work! This looks promising, I shall have a look soon. It's still on my list. :)

@emri99
Copy link

emri99 commented Jul 7, 2017

@SanderSander Great job!

Currently testing, it seems that it does not work anymore with path wildcards.

Wrote a small fix here, however it changes the way Studio Config reads the path by extending wildcards to existing folders containing "composer.json".

@SanderSander
Copy link
Author

SanderSander commented Jul 27, 2017

Nice! I have some time this weekend to work in this.

  • studio load [path] and studio unload [path] do work, but you have to do an composer update to create the actual symlinks. This isn't always desired and the studio command should create the symlinks immediately.

Then I think this PR is complete.

Some other points (outside this PR)

  • small thingy currently there is a src/Filesystem but we use the composer filesystem which can be a bit confusing.
  • Furthermore I want this integrated in my PHPStorm/IDE 😎

@tomicakorac
Copy link

Any hope that this fix gets merged?

@franzliedke
Copy link
Owner

I am about to leave for vacation, but I promise that I will have a look when I return (in two weeks).

@tomicakorac
Copy link

So, still nothing @franzliedke ? Is there any known issue with this PR that's blocking the merge?

@nathanmerrill
Copy link

@franzliedke +1 from me as well. It's already been 2 weeks :)

@franzliedke
Copy link
Owner

Hi folks, sorry for the long absence. I've just released with a new version that takes care of other small issues / pull requests.

Now for the elephant in the room...

I just pulled this PR down into a local branch and will play around with it. Thanks for the awesome work, @SanderSander! I will try to turn this into a beta release quickly and all of you will have to try it out. 😉

@spotman
Copy link

spotman commented Jan 16, 2018

👍 for merging this!

@rask rask mentioned this pull request Jan 29, 2018
@omacesc
Copy link

omacesc commented May 16, 2019

@franzliedke any news about this? thanks for your work, helps a lot in our work!

@omacesc
Copy link

omacesc commented Jun 14, 2019

I created a repo with the last version of studio and the solution by @SanderSander
https://github.com/omatech/studio

@scottsb
Copy link

scottsb commented Jan 2, 2021

@franzliedke @apfelbox We would love to use this project, but the manipulation of the composer.lock file is a major barrier right now. I'm excited to see further development here with the latest 0.15.0 release. Any hope of getting this fix merged in? (I see that a Git conflict has arisen; I could be interested in helping to resolve this, but I'd like to know if it's realistic that it would get considered for release in the reasonably near future.)

@apfelbox
Copy link
Collaborator

Hey @scottsb,

after reading #52, I think what studio should do is to leave the composer.lock like it would have been without using studio.
So while we need to keep the reference like it is now (as we installed a specific commit), we should definitely leave the original type + url.

I will try to have a look at this PR. 👍

@scottsb
Copy link

scottsb commented Jan 15, 2021

@apfelbox Maybe I'm not following, but it seems like modifying the reference without modifying the type/url could be a recipe for breaking the project if the partially modified composer.lock file is then committed. That's because there's no guarantee that the reference checked out locally is actually available from the source specified by the type/url.

@apfelbox
Copy link
Collaborator

Yeah, but that would be intended.

Because if you have a reference locally, that is not in the upstream repository, and you try to install this somewhere else, it should properly fail with "hey, I can't find this reference. Something is broken".

That is actually really important, because "just ignore the missing reference" is a hard break: your project depends on code from your studioed-library that won't exist when installing your project somewhere else. This will break hard in production.

The right thing to do is to fail as early as possible – and that is in composer install.

Just think of it this way: your code apparently requires the code from this reference, so installing it with any other version would invalidate the complete package management from composer.

@scottsb
Copy link

scottsb commented Jan 15, 2021

If the goal is to represent in the host project the version of the package as it exists locally in development, then there's another issue even if it can find the reference: you now have a lock file that may be inconsistent with the version constraint in composer.json, in a non-obvious way. (Composer won't warn about the lock being out of date because that's based on a hash of the composer.json file, which hasn't changed.)

Also, even updating the reference in the lock file is not a guarantee that the host package will have the right version of the code, because the reference is only the version that was installed when studio set up the symlink. It won't be automatically updated for further commits in the package (much less uncommitted work).

My thought is that studio ought to serve as a way to "disconnect" a package from Composer so that you can work on a package within a host environment. So long as it's disconnected, the host project's Composer locks should not be updated. Then, when you are ready to release an update to the package, you can tag it and release it. Then you would tell Composer to do a typical composer update to update the lock file per the version constraint.

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

Successfully merging this pull request may close these issues.

10 participants