phar-io / phive

The Phar Installation and Verification Environment (PHIVE)
https://phar.io
BSD 3-Clause "New" or "Revised" License
581 stars 45 forks source link

Add ".phar" extension to symlink files #116

Open oqq opened 7 years ago

oqq commented 7 years ago

Unfortunately PHPStorm is not able to work out if a symlink is spotting to a phar file. In my case this removes the autocomplete feature for unit tests, since PHPStorm doesn't know about PHPUnit at all, although the phar file symlink is under the project root.

To fix this behavior I have to add the ".phar" extension in the phive.xml manually.

How about to add the extension by default or at least with a global config paramter?

tommy-muehle commented 7 years ago

Can confirm this. Maybe an additional flag "--with-suffix" would solve this problem. By default this option should be false.

theseer commented 7 years ago

I agree that to be annoying.

Instead of only controlling the potential extension though, I'd prefer to have a switch to override the complete filename of the destination file. We already have --target to control the path, so maybe we should add an --as which will take a filename (no path)?

Sidenote: This has nothing to do with symlinks. PHPStorm as of 2017.2.4 does not handle files without an extension very well, regardless of being a symlink or not. Funny enough, if the phar is symlinked it at least is detected as PHP while a copy of the same file is just an unknown file.

Sidenote 2: I consider that a bug in PHPStorm rather than an issue of phive. But until Jetbrains decides to fix that in PHPStorm, phive probably needs to offer a workaround.

ThomasWeinert commented 7 years ago

PHPUnit expects its plugins to have a .phar extension as well. But for compatibility the tools should be callable by name (without extension).

I suggest using a combination of the two approaches:

The tool.phar would be available for PHPStorm and PHPUnit, while the shell script allows to call the tool on the console or in a build file with just the name.

theseer commented 7 years ago

We already considered simply creating two links or have the suggested shell wrapper, but decided against doing in the proposed way.

Let me elaborate that a little: PHPUnit is technically an edge case, as it's a tool and a framework/library. For pretty much every other tool you exactly do NOT want its code to show up in the IDE's auto completion, so PHP Storm's limitation actually turns out to be a feature ;-)

Phive is currently lacking support for anything but "application" type phars (as per manifest) - meaning we don't yet make use of the information provided by the manifest. The plan is to add support for the other types and have extensions as well as libraries keep the .phar extension and also install them in dedicated directories. That way, the tools directory can be ignored from the IDE while phars in the the library and extension directories can still be included.

Regaring PHPUnit: We're currently considering to extend the manifest definition to allow something like a multi-type archive, say application and library, which would then cause Phive to create two links, one without extension in tools and one with extension in library - or whatever folder name we decide as the default.

Or, pending a discussion with @sebastianbergmann, we might add a new stub type and have a PHPUnit-stub-phar that is logically linked to the application type phar. The idea here obviously is to have only the stubs for the IDE in the stub-phar and to only include the stubs for the "public" API of PHPUnit and accompanying classes. That way, code that is internal to PHPUnit or would otherwise be potentially duplicated (like say symfony libs) will not show up as duplicate in PHP Storm.

ThomasWeinert commented 7 years ago

Handling the extension phars for a tool in a separate way would be the best solution.

I am playing around with hardlinks for the phars at the moment, trying to solve the PHPStorm issue on Windows. https://github.com/ThomasWeinert/phive/commit/a19bf71815ba71bf0d4fbc9db4ffaff82bc1af61

theseer commented 7 years ago

That looks interesting :)

ThomasWeinert commented 7 years ago

I opened a merge request for it. However it might make sense to move some logic as options into the PharActivator implementations. Maybe something like:

$activator->activate($source, $destination, PharActivator::FORCE_COPY | PharActivator::EXECUTABLE);

PharActivator::FORCE_COPY - copy the phar to the destination, not just link it. PharActivator::EXECUTABLE - create a shell script, symlink or whatever that allows to call tools\tool

A standard tool just needs to be available as tools\tool. Some tools need to be available as phars for integration. And some extensions for tools are just need to be available as phar, but not executable.

That seems to be difficult to implement OS independent.

ThomasWeinert commented 6 years ago

After using it for I while I have some more feedback. It is possible to exclude/include single Phars in PHPStorm.

exclude-phar

The Phar setup in PHPStorm is really convenient - its always /projectpath/tools/tool.phar. Updates and versions are handled by Phive.

DaGhostman commented 6 years ago

I think it is better to have the default approach as it is now (no extensio) as it is a pretty common convention for linux/mac environments but provide 2 flags possibly:

The second one is more or less IMO a convenience one, to not force the full name of a package (useful with long ones)

ThomasWeinert commented 6 years ago

Maybe not a flag, but an configuration option. Phive does not have that, so that would be a bigger change.

Between two contributors of a project one could have Phive configured to provide local Phars for IDE integration and the other not.

theseer commented 6 years ago

Imho, there is literally no point in having a tool's classes show up in an IDE. Rather to the contrary: it's highly annoying if that happens and getting rid of these was one of the reasons to start phive.

For the aforementioned reasons and conceptual ideas, I'm against adding a switch that adds an extension to the tool filename. I also fail to see the point of adding a configuration option? What would be the benefit?

ThomasWeinert commented 6 years ago

I was not speaking about code completion, but IDE integration. PHPStorm has plugins that integrate the tools, so that you can use them via GUI and internal functions.

Currently Phive allows to reference the execute the tools using an predefined stable comment, independent from the version - but only via console command. But to use them from the PHPStorm you have to configure something like: ~\.phive\phars\phpunit-6.4.3.phar. This is really fragile and it can easily lead to inconsistencies or breaks because of version changes.

That is why I modified Phive to provide me with local Phars. I exclude them from the code completion, but I get a stable filename for the IDE settings. The setup for the tools gets a lot easier and profits from version management. It makes my live a lot easier.

theseer commented 6 years ago

Maybe it's too warm, but I don't get it.

Why would you even consider to reference anything the local storage path? That's phive's internal domain and should be an implementation detail.

With phive install you get a stable per project symlink/copy or even a global install (in /usr/bin for instance).

ThomasWeinert commented 6 years ago

Maybe this is only needed on Windows. I don't get a local symlink.

On windows the tools directory only includes bat files that call the Phar inside the local storage path or they contain a copy of the Phar without the extension.

theseer commented 6 years ago

Why can't windows just be a useful OS?

Okay, I keep forgetting that windows is incapable of properly handling symlinks and still relies on file extensions.

But then, why would the IDE have issues with running a tool that way? You don't tell me the plugin actually checks the content of the file you point it to or its extension?

ThomasWeinert commented 6 years ago

The file, PHPStorm seems to validate the file and extract the version.

theseer commented 6 years ago

Just to verify with PHPUnit:

screenshot-20180507154325-2266x1519

Works for me on Linux with symlinks, but doesn't work when wrapped in a shell script:

screenshot-20180507154945-2266x1305

Not that that behaviour makes any sense to me though.

Edit: Okay, it does make sense. They probably run $configured-php ./phpunit.sh --version which of course doesn't work.

ThomasWeinert commented 6 years ago

Putting the *.phar files into the tools directory has the downside that an IDE might recognize it as part of the project code. But it will allow for IDE integration on Windows and code completion for unit tests.

The best solution for PHPUnit would be a special Phar that contains the public API. But this sounds complex and a lot of work in Phive and PHPUnit. It will take time. Even it that is solved here would still be the problem of IDE integration on Windows (Well, it would be possible to use a different file extension).

That is why I suggest moving the decision to the user for now. With a global option / command argument in Phive the individual user could decide if he/she wants locally linked Phars.