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

Relative Path File Collection #8035

Closed
Caffe1neAdd1ct opened this Issue Mar 13, 2019 · 3 comments

Comments

Projects
None yet
2 participants
@Caffe1neAdd1ct
Copy link

Caffe1neAdd1ct commented Mar 13, 2019

I'm working on an interesting issue when using the COMPOSER_VENDOR_DIR environment variable, however the patching library netresearch/composer-patches-plugin expects patch files to be listed from the project root relatively, for example:

"extra": {
        "patches": {
            "other/dep": {
                "v6.5.4": [
                    {
                        "title": "[BUGFIX] fixes a bug in other/dep/thing.php",
                        "url": "vendor/me/patches/src/other/dep/thing.patch"
                    }
                ]
            }
        }
    }

I've noted the patching library uses src/Composer/Util/RemoteFilesystem.php getContents():

	public function getContents($url) {
		if (array_key_exists($url, $this->cache)) {
			return $this->cache[$url];
		}
		return $this->cache[$url] = $this->remoteFileSystem->getContents($this->getOriginUrl($url), $url, false);
	}

$this->remoteFileSystem being the RemoteFilesystem instance.

As not everyone will be changing the vendor path, I can't specify different paths in the patch library composer.json e.g:

"extra": {
        "patches": {
            "other/dep": {
                "v6.5.4": [
                    {
                        "title": "[BUGFIX] fixes a bug in other/dep/thing.php",
                        "url": "other-vendor-folder/me/patches/src/other/dep/thing.patch"
                    }
                ]
            }
        }
    }

I've got two options:

  1. Request a change to src/Composer/Util/RemoteFilesystem.php to translate any paths starting with vendor/ to COMPOSER_VENDOR_DIR . '/' which might not be wanted and or might break things (though worth asking the question)

  2. Request a change to netresearch/composer-patches-plugin in their getContents method to make this translation.

So my question is would it be possible for composer to automatically translate requests for files starting 'vendor/' to use the COMPOSER_VENDOR_DIR environment variable when requesting local filesystem file content?

Any other solutions or recommendations more than welcome 👍

@alcohol

This comment has been minimized.

Copy link
Member

alcohol commented Mar 15, 2019

This seems like logic that does not belong in the getContents() method. So I would say that the answer to your question is a definite "no".

@alcohol alcohol added the Question label Mar 15, 2019

@Caffe1neAdd1ct

This comment has been minimized.

Copy link
Author

Caffe1neAdd1ct commented Mar 15, 2019

@alcohol Cheers for the review!

I did see a lot of mapping for hosts and other logic in there and wondered if it would be a useful addition, looking at it from a composer point of view, it deals with the paths fine and shouldn't need any mappings in the download/getContents method.

So the correct solution would be the library/plugin correctly resolving the vendor path, e.g: https://github.com/netresearch/composer-patches-plugin/pull/30/files

Just wanted to make sure the change was applied to the best possible place 👍

@alcohol

This comment has been minimized.

Copy link
Member

alcohol commented Mar 15, 2019

I think it would be safer indeed if the plugin respected the environment variables that Composer supports.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.
You signed in with another tab or window. Reload to refresh your session. You signed out in another tab or window. Reload to refresh your session.