PHAR version requires "_HumbugBox" prefixed implementation of ProviderInterface

rieschl commented 4 years ago

Bug Report

Q	A
BC Break	no
Version	2.9.0

Summary

It seems that the packaging to a PHAR file does some monkey business with the namespace of the project. The provider has to be an instance of _HumbugBoxaa7bf30d75ed\Phly\KeepAChangelog\Provider\ProviderInterface.

Current behaviour

When releasing a version with keep-a-changelog version:release 1.2.3 an error is raised: The provider as currently configured is incapable of performing a release. Problem is in the config file the provider class is Phly\KeepAChangelog\Provider\GitHub whereas the PHAR wants an implementation of _HumbugBoxaa7bf30d75ed\Phly\KeepAChangelog\Provider\ProviderInterface.

How to reproduce

Download the PHAR file, create a global config with keep-a-changelog config:create -g, set the GitHub token and try to release a version. The error above is raised. When changing the provider to _HumbugBoxaa7bf30d75ed\Phly\KeepAChangelog\Provider\GitHub it works as expected.

Expected behaviour

The _HumbugBoxaa7bf30d75ed namespace shouldn't be there.

It seems every packaging creates a random string after the prefix _HumbugBox. I also tested it with version 2.8.1. The namespace is _HumbugBoxdcb0919ddeeb there.

weierophinney commented 4 years ago

@heiglandreas Have you run into this? Any ideas on how to fix it?

rieschl commented 4 years ago

After a bit of digging I think that the compactor does this for PHAR code isolation. But I'm not sure if this is required or can be worked around.

Funny sidenote: Apparently, box itself has this "issue" in their PHAR files. I played around with it a bit and because of an error message in box compile I executed box compile -v and got this:

PHP Fatal error:  Uncaught Error: Call to undefined function _HumbugBox48cf944fc33a\Symfony\Component\Console\get_debug_type() in phar:///home/neo/bin/box/vendor/symfony/console/Application.php:523
Stack trace:
#0 phar:///home/neo/bin/box/vendor/symfony/console/Application.php(516): _HumbugBox48cf944fc33a\Symfony\Component\Console\Application->doActuallyRenderThrowable()
#1 phar:///home/neo/bin/box/vendor/symfony/console/Application.php(491): _HumbugBox48cf944fc33a\Symfony\Component\Console\Application->doRenderThrowable()
#2 phar:///home/neo/bin/box/vendor/symfony/console/Application.php(84): _HumbugBox48cf944fc33a\Symfony\Component\Console\Application->renderThrowable()
#3 phar:///home/neo/bin/box/vendor/symfony/console/Application.php(104): _HumbugBox48cf944fc33a\Symfony\Component\Console\Application->_HumbugBox48cf944fc33a\Symfony\Component\Console\{closure}()
#4 phar:///home/neo/bin/box/bin/box(41): _HumbugBox48cf944fc33a\Symfony\Component\Console\Application->run()
#5 /home/neo/bin/box(18): require('phar:///home/ne...')
 in phar:///home/neo/bin/box/vendor/symfony/console/Application.php on line 523

😄

So, maybe it's a feature, not a bug?

weierophinney commented 4 years ago

I just discovered the same regarding code isolation, @rieschl .

However, I tried locally building the phar without the code isolation compactor, and it appears to work fine. As such, I'm going to create a patch and do a release around that, as that version appears to fix the issues in this and #82.

Can you do me a favor, and do the following please?

Clone the phly/keep-a-changelog repo
Run composer buildphar
Use the build/keep-a-changelog.phar file in the project you're using that has a ProviderInterface implementation present

and then report to me if there are still issues. I don't think there will be, but I am definitely curious.

weierophinney commented 4 years ago

Oh, important note: remove line three of the box.json.dist file (the one referencing "compactors"), and then remove the trailing comma from the preceding line BEFORE running composer buildphar.

heiglandreas commented 4 years ago

That is actually a feature. It is a so called "scoped PHAR" (I blogged about that). The issue is that the scope-namespace needs to be added to the config file...

I'll see whether that can automagically added...

heiglandreas commented 4 years ago

We probably need to whitelist the Providers as described in the PHP-Scoper docs...

rieschl commented 4 years ago

It is a so called "scoped PHAR"

Right, I also discovered that in the meantime 😄

We probably need to whitelist the Providers as described in the PHP-Scoper docs...

I played around a bit with that. Whitelisting the Interface or the implementation doesn't solve the problem. It's probably also necessary if we want to keep the scoped PHAR. Whitelisting scopes the class but also adds a class_alias for the original FQCN.

The problem is the scoping of src/Provider/ProviderSpec.php. It is changed from:

    public function isComplete(): bool
    {
        return $this->className
            && class_exists($this->className)
            && in_array(ProviderInterface::class, class_implements($this->className), true);
    }

to

    public function isComplete(): bool
    {
        return $this->className && class_exists($this->className) && in_array(
                \_HumbugBoxa35b2ab0d726\Phly\KeepAChangelog\Provider\ProviderInterface::class,
                class_implements($this->className),
                true
            );
    }

Because of that the check fails if the _HumbugBox prefix isn't in the config file. Whitelisting the file (files-whitelist) also doesn't work because then the Class isn't found anymore. I could think of some tricking the scoper so it's not rewritten (e.g. put the namespace parts into an array and implode('\\', $parts) it.)

Can you do me a favor, and do the following please?

* Clone the phly/keep-a-changelog repo
* Run `composer buildphar`
* Use the `build/keep-a-changelog.phar` file in the project you're using that has a `ProviderInterface` implementation present

Yes, removing the scoper solves that issue and also #82. But I'm not the one to decide to remove the scoping of the PHAR file. Probably there won't be an issue if we remove the scoping because the PHAR doesn't interact with other classes of the project it is used in. But I haven't used box-project or php-scoper before so I could be wrong. I'd be happy to create a PR if you want.

weierophinney commented 4 years ago

@heiglandreas So, I see three separate issues, then:

Some users may want to extend the functionality of keep-a-changelog by supplying custom providers. I'd argue that for those users, we can probably stipulate that this functionality only works with a global or local composer install, and/or a clone-based install.
Our configuration uses class names to indicate which provider should be used. If scoping changes those names and/or changes inheritance, this can cause an inability to use configuration when using the PHAR.
When generating configuration, I would expect that doing so from the PHAR file would be identical. However, per #82, it looks like there's a value being injected before the first section token, which thus makes the configuration unusable. (I've verified this with the 2.9.0 phar file).

Locally, I checked to see if I could resolve either issue, in a variety of ways.

First, removed the scoping compactor entirely, and all problems were resolved.

Second, I tried adding a scoper.inc.php that whitelisted the provider namespace, using the string Phly\KeepAChangelog\Provider\*. This left us in the same state as having if we just had the scoper compactor without configuration.

Third, I tried whitelisting the various classes in that same namespace individually. This resolved the second issue, but not the third.

Looking into the third, I tried whitelisting the Phly\KeepAChangelog\ConfigCommand\CreateLocalConfigListener class. This did not work. At that point, I extracted the phar locally so I could look at files. Of interest was the aforementioned class, and its sibling, CreateGlobalConfigListener. Each has a TEMPLATE constant they declare, with roughly the same contents (the second file also provides entries for tokens for each of the github and gitlab providers). However, the CreateGlobalConfigListener class does not have the generated unique Humbug namespace present; only the CreateLocalConfigListener class does.

As an experiment, I tried changing CreateLocalConfigListener::getConfigTemplate() to return a nowdoc, instead of the class constant. This, too, got injected.

The scoper documentation indicates that certain strings that reference classes can be rewritten, but it seems arbitrary that one and not the other is not rewritten in this case. This makes me think #82 may actually be a bug with either the Box project or the PHP-Scoper project.

I'm going to continue looking into this, but if either of you have ideas, please share!

phly / keep-a-changelog