Skip to content

puli cli is now required by discovery #267

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

Merged
merged 1 commit into from
Jan 4, 2016
Merged

puli cli is now required by discovery #267

merged 1 commit into from
Jan 4, 2016

Conversation

dbu
Copy link
Contributor

@dbu dbu commented Jan 4, 2016

No description provided.

@dbu dbu added this to the 2.0 milestone Jan 4, 2016
@dbu
Copy link
Contributor Author

dbu commented Jan 4, 2016

looks like puli is not working as it should. @sagikazarmark could you please have a look and tell us what we do wrong?

@dbu
Copy link
Contributor Author

dbu commented Jan 4, 2016

@sagikazarmark
Copy link

@webmozart can this be an incompatibility between beta versions?

@webmozart
Copy link

The current development build is broken. I recommend to use the beta versions for better stability. The next beta should arrive by mid January.

@sagikazarmark
Copy link

Locking "puli/discovery": "1.0.0-beta8" solves the problem. Is that incompatible with Symfony 3.0?

@dbu
Copy link
Contributor Author

dbu commented Jan 4, 2016

https://github.com/puli/discovery/blob/1.0.0-beta8/composer.json does not mention symfony so we should be good.

@sagikazarmark
Copy link

Where was the problem in the first place? @Nyholm provided a PR for symfony 3.0 support. Should we add the lock back to discovery, or override in FOS HTTP Cache for the time being?

@dbu
Copy link
Contributor Author

dbu commented Jan 4, 2016

the symfony 3 problem was with puli/cli. i try to fix stuff here: php-http/discovery#40

i have no idea why composer is ignoring the prefer-stable: true flag - otherwise it should install discovery beta8 and things would not fail here.

@webmozart
Copy link

Sorry for the problems guys :( The next beta release is compatible with Symfony 3.0 and should fix a lot of your problems.

@sagikazarmark
Copy link

Thanks @webmozart

@Nyholm
Copy link
Contributor

Nyholm commented Jan 4, 2016

puli/discovery is compatible with sf3. It is php-http/discovery that is not compatible with sf3. Because:

php-http/discovery => puli/cli => padraic/phar-updater => symfony/finder:~2.2

Here is the PR to fix that issue: humbug/phar-updater#16

@dbu
Copy link
Contributor Author

dbu commented Jan 4, 2016

@webmozart why does puli/cli depend on phar-updater? should that not be a require-dev for when you build the phar? or can the phar also be built out of any project that installed puli/cli into vendor? but even then, should phar-updater not be optional? i guess there are rarely cases where one would want to build the phar out of anything but a clone of the puli/cli repository directly.

@stof
Copy link
Member

stof commented Jan 4, 2016

@dbu a phar is meant to be built without dev requirements (you don't want to include PHPUnit in them). So making it a requirement is logical.

@dbu
Copy link
Contributor Author

dbu commented Jan 4, 2016

okay. but the phar-updater is irrelevant for the cli when used in vendor. it feels like the phar packaging stuff and some of puli's application logic got mixed together in one repository. we certainly don't need the phar updater here (and it creates problems because of dependencies).

@sagikazarmark
Copy link

I guess the phar updater package provides the self-update command which is used in the cli.

An alternative solution would be removing the update command from the cli package itself (as it doesn't makes sense there) and create a cli-phar package, require phar-updater there and add the update command there. Not sure though wether it worths the hassle, but if installing cli is a viable option, then we don't need self-update inside that.

@webmozart
Copy link

I don't quite understand the point of this discussion. Isn't humbug/phar-updater#16 fixing this issue?

@stof
Copy link
Member

stof commented Jan 4, 2016

@dbu the question IMO is why you need to install the CLI in your vendors. The CLI is meant to be its own project AFAIK.

@webmozart
Copy link

@stof It can be either. You can either install the PHAR globally or require the CLI in your project.

@sagikazarmark
Copy link

@webmozart yes, it does. I think it is just a theoretical debate about the purpose and usage of CLI package, at least indirectly.

@stof One reason is that installing cli is easier than saying have it installed in your prefix. I guess, one day, puli will be installed on every developer machine...but this is not that day (yet).

@dbu
Copy link
Contributor Author

dbu commented Jan 4, 2016

I don't quite understand the point of this discussion. Isn't humbug/phar-updater#16 fixing this issue?

this is fixing the current problem at hand. but as mark says, i question why we would have that dependency in general. the problem at hand illustrates one of the issues of having an "unnecessary" dependency. phar-updater could one day conflict with something that nothing we or puli use in the non-phar usecase have a conflict with, or random other problems. and its confusing for users of FOSHttpCache why it ends up installing a phar-updater...

@dbu
Copy link
Contributor Author

dbu commented Jan 4, 2016

php-http/discovery#41 makes me wonder wheather we really want discovery included by default... its dependencies are indeed pretty massive.

@joelwurtz
Copy link
Contributor

@dbu IMO, discovery should always be a suggested dependancy when user want a very friendly experience. But for advanced user it does not make any sense IMO.

The debate here, is what do you want most ? : something nice with a friendly experience, or something robust with more documentation ?

@sagikazarmark
Copy link

One possible way of making discovery optional is adding an additional check if the desired discovery class exists. If not, fall back to exception.

@sagikazarmark
Copy link

Let's move the CLI part of this discussion to php-http/discovery#41

dbu added a commit that referenced this pull request Jan 4, 2016
puli cli is now required by discovery
@dbu dbu merged commit 8304ca5 into master Jan 4, 2016
@dbu dbu deleted the puli-cli branch January 4, 2016 16:17
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.

6 participants