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

Stop providing a phar #167

Closed
mglaman opened this issue May 11, 2020 · 16 comments · Fixed by #183
Closed

Stop providing a phar #167

mglaman opened this issue May 11, 2020 · 16 comments · Fixed by #183

Comments

@mglaman
Copy link
Owner

mglaman commented May 11, 2020

TL;DR phpstan 0.12 support broke packaging as a phar, install as a project dependency or globally via Composer


Supporting PHPStan 0.12 caused the removal of PHPStan's source code, leaving only the built and namespaced. We have to call the PHPStan phar directly. This does not work when using drupal-check as a phar.

php phar:///path/to/drupal-checker.phar/vendor/phpstan/phpstan/phpstan

This file cannot be executed. PHP can detect it and load it, but nothing outside of the phar. As far as I can tell we cannot run it with PHP.

We had to switch to running PHPStan as a process instead of directly loading the analyzer class.

        $process = new Process($command);
        $process->setTty(Tty::isTtySupported());
        $process->setTimeout(null);
        $process->run(static function ($type, $buffer) use ($output) {
            if (Process::ERR === $type) {
                $output->write($buffer, false, OutputInterface::OUTPUT_RAW);
            } else {
                $output->writeln($buffer, OutputInterface::OUTPUT_RAW);
            }
        });

This also means none of the includes for configuration work, either.

@mglaman
Copy link
Owner Author

mglaman commented May 11, 2020

Created a Twitter poll: https://twitter.com/nmdmatt/status/1259687829904465920

@hussainweb
Copy link

I responded on Twitter and then realised I could do that here without the restriction of 280 chars.

Currently, I am using version 1.0.14 in my https://github.com/hussainweb/drupalqa Docker image. I would like to continue to make it easy for people to use this via the image. If phar isn't going to be supported, what other methods could work? Thanks.

@mglaman
Copy link
Owner Author

mglaman commented May 11, 2020

😄 I'll copy my reply here, too.

1.0.x works because it's phpstan 0.11
1.1.x is new phpstan, new phar problems.

The fix would be to include phpstan-src and directly call internal classes again, and basically build our own compiled version of phpstan. So drupal-check would become a bespoke compilation of phpstan. Which could be good, but also raises the maintenance. Depends if this tool is still valuable to the community.

@mglaman
Copy link
Owner Author

mglaman commented May 11, 2020

@hussainweb do you have usage states for your drupalqa image? I just want to determine the level of effort to custom compile phpstan-src (basically recreating the current phpstan/phpstan repo) versus pdocumenting ways folks can configure phpstan with the normal defaults this provides.

Because the drupalqa image could provide phpstan and point to a default configuration file that is shipped.But I think it would still require the Drupal project to have the proper PHPStan extensions installed, which this tool mitigates.

@arderyp
Copy link

arderyp commented May 11, 2020

i've had success installing it as a dependency of my drupal project, but that's only because installing via composer and also building from source both failed me.

@ondrejmirtes
Copy link

The fix would be to include phpstan-src and directly call internal classes again, and basically build our own compiled version of phpstan.

You never want to do that, it’s really complex and my job as PHPStan creator to do this.

@ondrejmirtes
Copy link

This option is something I’d get behind as it’s meant to be used like that: https://twitter.com/nmdmatt/status/1259688161149628417?s=21

@hussainweb
Copy link

@mglaman, unfortunately, I don't have any usage stats except total number of downloads (7.7k). It feels to me that the "custom compile phpstan-src" is more trouble than it's worth. If, in the long run, developers are expected to require-dev phpstan and other packages, I'd go with that. That would probably mean that there is no point bundling drupal-check in the drupalqa image because there is nothing to bundle (is there?)

I still feel that it makes sense for a tool like this to live globally with config per project. At the same time, I don't want to make it extremely hard to maintain.

@ondrejmirtes
Copy link

At the same time, PHAR distribution of PHPStan means there are zero dependencies so everyone should be able to install it without risking conflicts.

@hansfn
Copy link

hansfn commented May 11, 2020

I support 100% that you no longer provide a phar.

As a Drupal developer, you are (forced?) to use Composer as one of your main tools. Hence, it's no additional cost for your users to install drupal-check using Composer - either site-local or globally.

Until Composer is fixed, we should avoid “composer global require”, but rather use cgr. (The reason so few people are voting for cgr is that they don't know about it or don't understand the problems with "composer global require”.) Using cgr also "means there are zero dependencies so everyone should be able to install it without risking conflicts".

Finally, I dislike phar installation since you no longer have easy access to the files - and the installation is manual.

@mbomb007
Copy link

mbomb007 commented May 11, 2020

I'd never heard of cgr. Here's a link for those who are similarly new to it: https://github.com/consolidation/cgr

The Composer global require command is a recommended installation technique for many PHP commandline tools; however, users who install tools in this way risk encountering installation failures caused by dependency conflicts between different projects. The cgr script behaves similarly to composer global require, using composer require to install a global-per-user copy of a commandline tool, but in an isolated location that will not experience dependency conflicts with other globally-installed tools.

@ondrejmirtes
Copy link

I recommend “composer require” as the preferred approach so that PHPStan version is shared for all the developers and the CI build is reproducible.

Classic “composer global require” is also fine because PHPStan is distributed as PHAR even inside Composer package so there’s no risk of dependencies conflict.

@mglaman
Copy link
Owner Author

mglaman commented May 11, 2020

@ondrejmirtes yeah, in #172 I consider deprecating this tool for just PHPStan. Earlier I was borrowing from larastan which wrapped PHPStan internals.

A goal here was to reduce the setup for requiring the Drupal extension and the deprecation rules and only running deprecation checks.

Maybe drupal-check isn't completely deprecated, but it really shouldn't be considered a standalone CLI. It could be used to generate PHPStan configuration and then folks just run ./vendor/bin/phpstan

@ondrejmirtes
Copy link

@mglaman Larastan also switched to be "just another" PHPStan extension.

@mglaman
Copy link
Owner Author

mglaman commented May 11, 2020

@ondrejmirtes that's what I thought. So I'll formulate a plan to push phpstan-drupal more directly, I even have https://github.com/mglaman/phpstan-drupal-deprecations which provides generic rules to kickstart testing (made in the early days.)

So I'll stop shipping the Phar. And move towards "hey, let's just use PHPStan directly" and help the Drupal community find ways to migrate that way. Given some of the issues I've seen in this queue, quite a few have moved that way anyways.

jebschiefer added a commit to Modea/drupal-check-docker that referenced this issue May 28, 2020
mglaman added a commit that referenced this issue Jul 29, 2020
Remove building and usage of the Phar
mglaman added a commit that referenced this issue Jul 29, 2020
@mglaman mglaman unpinned this issue Jul 29, 2020
jacine added a commit to jacine/vscode-drupal-check that referenced this issue Aug 28, 2020
hussainweb added a commit to hussainweb/drupalqa that referenced this issue Nov 7, 2021
drupal-check has long removed the PHAR support. It had stopped working since version after 1.0.14 and so we stuck to that version. Now, we can install this using jakzal/toolbox (really, just composer bin plugin) and drupal-check can now live in its own namespace.

More details at mglaman/drupal-check#167 (comment)
@hussainweb
Copy link

FWIW, I figured out a way to put drupal-check in my drupalqa image. Since the PHAR support is gone for good, I am now using composer to install this. The difference is that I am using the composer bin plugin to isolate it from the rest of the tools. I do this using the jakzal/toolbox which is available from the base image I use. Details in this commit: hussainweb/drupalqa@dea27eb

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 a pull request may close this issue.

6 participants