Skip to content
This repository has been archived by the owner on Sep 16, 2021. It is now read-only.

[POC] Copy used Puli classes in this component to avoid unstable deps #39

Merged
merged 8 commits into from
Jan 22, 2017

Conversation

wouterj
Copy link
Member

@wouterj wouterj commented Dec 26, 2016

We can't release any stable version of these package and its friends because Puli is not yet stable. As we want to get CMF 2.0 out very soon, I propose to move Puli-light to this component.

I've marked the component as internal, people should not yet use it. Once Puli is stable, the puli copied classes will be removed and we'll depend on Puli and its classes once again. This can be done in a 1.x version and wouldn't change any output from the ResourceRestBundle.

@symfony-cmf/core what do we think about this?
@webmozart As you're the owner of Puli, I would like to have your opinion on this one as well. I've copied puli's LICENSE file and file headers in this project. Should I do something more?

@wouterj wouterj added this to the 1.0 milestone Dec 26, 2016
@dantleech
Copy link
Member

Yes, this is what I was thinking. There is not much to copy.

@dantleech
Copy link
Member

This will prevent the FilesystemRepository being used with this component?

@wouterj
Copy link
Member Author

wouterj commented Dec 28, 2016

@dantleech yes. If we copy the FilesystemRepository + related classes to the component, the result would be copying almost all of Puli.

Copying Puli is done for one reason: Allowing us to release a very minimal 1.0.0, so we can release tree browser bundle and CMF packages stable. So I decided we only need the minimal Puli required to get the tree browser bundle working.

@dbu
Copy link
Member

dbu commented Dec 30, 2016

i wonder if this is better than depending on puli 1.0@beta (or even lock to beta10). the upgrade path to a stable puli is quite a BC break. whereas if we depend on beta puli, it would potentially be a non-issue, or small changes.

but if we think that is too wobbly, then lets do this and release. we can always release a new version later - its clear enough in the code that this is a temporary solution until puli is released.

@wouterj
Copy link
Member Author

wouterj commented Jan 2, 2017

@dbu yeah, maybe locking Puli at 1.0.0-beta10 is a better solution indeed.

@dantleech
Copy link
Member

i wonder if this is better than depending on puli 1.0@beta

Would that mean that it would be uninstallable for packages with minimum-stability: stable ?

@dbu
Copy link
Member

dbu commented Jan 2, 2017

as long as what we depend on does not in turn depend on other non-stable packages, having the dependency with @beta works even with minimum-stability: stable. the @beta overrides the global stability setting.

@dantleech
Copy link
Member

dantleech commented Jan 2, 2017

I do remember having to explicitly declare puli/respository@beta in addition to this package, otherwise it would fail to install, .g. https://github.com/psiphp/resource-browser/blob/master/composer.json#L13

@dantleech
Copy link
Member

e.g.

{
    "name": "dantleech/foo",
    "authors": [
        {
            "name": "dantleech",
            "email": "[email protected]"
        }
    ],
    "require": {
        "symfony-cmf/resource": "dev-master"
    }
}
$ composer install
You are running composer with xdebug enabled. This has a major impact on runtime performance. See https://getcomposer.org/xdebug
Loading composer repositories with package information
Updating dependencies (including require-dev)
Your requirements could not be resolved to an installable set of packages.

  Problem 1
    - Installation request for symfony-cmf/resource dev-master -> satisfiable by symfony-cmf/resource[dev-master].
    - symfony-cmf/resource dev-master requires puli/repository ^1.0.0-beta10 -> satisfiable by puli/repository[1.0.0-beta10, 1.0.x-dev] but these conflict with your requirements or minimum-stability.

@dbu
Copy link
Member

dbu commented Jan 3, 2017

but ^1.0.0-beta10 is not ^1.0.0-beta10@beta - afaik the @ is what makes this work.

but i don't mind either way, maybe its better to simply wrap this up and merge the the needed classes

@@ -9,12 +9,13 @@
"homepage": "https://github.com/symfony-cmf/symfony-cmf/contributors"
}
],
"minimum-stability": "dev",
"minimum-stability": "RC",
Copy link
Member

Choose a reason for hiding this comment

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

"jackalope/jackalope-fs": "dev-master", no longer resolves with this stability requirement. can we switch to jackalope-doctrine-dbal (maybe with sqlite if that helps?)

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, will check that all tomorrow.

It doesn't matter which jackalope we install, as long as we install a
jackalope/jackalope-transport package.
@wouterj wouterj merged commit 59b55a3 into master Jan 22, 2017
@wouterj wouterj deleted the include-puli branch January 22, 2017 13:50
@wouterj wouterj removed the wip/poc label Jan 22, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants