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

Support package names that differ from repository names #31

Merged
merged 6 commits into from
Oct 2, 2017

Conversation

Sidsector9
Copy link
Member

@Sidsector9 Sidsector9 commented Aug 30, 2017

As of now, only packages hosted on GitHub is supported.

Related to issue 4232

@@ -202,6 +202,12 @@ public function install( $args, $assoc_args ) {
preg_match( '#([^:\/]+\/[^\/]+)\.git#', $package_name, $matches );
if ( ! empty( $matches[1] ) ) {
$package_name = $matches[1];
$raw_content_url = 'https://raw.githubusercontent.com/' . $package_name . '/master/composer.json';
$composer_content_as_array = json_decode( $this->get_data_from_url( $raw_content_url ), true );
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not use WP_CLI\Utils\http_request()?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My bad, I was unaware of this function 😓 I'll make the changes

Copy link
Member

@danielbachhuber danielbachhuber left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you include a test for this change, please? Generally, tests are required for all pull requests.

@Sidsector9
Copy link
Member Author

@danielbachhuber I wanted to discuss this feature before adding tests, as you can see this fix only works for packages hosted on GitHub which restricts Git URLs hosted on different remote repositories like Bitbucket and GitLab. Do you have a better idea which I could implement?

@danielbachhuber
Copy link
Member

I'll defer to @schlessera

@schlessera
Copy link
Member

@Sidsector9 There's already a function to get the package name: self::get_package_name_and_version_from_dir_package()
I'm not sure it makes sense to request the composer.json file twice, as in general, names should match up. The check should be done after the package has been normally loaded.
Also, I think the actual action to take when we detect a mismatch should be to fix it automatically, with a warning. To fix it automatically, the "repositories" entry that was created needs to be adapted, to match the correct package name (instead of the GitHub repo name).
Would you be willing to investigate that approach, of fixing the actual problem?

Repositories map package name to the repository location and require refers to package name.
@Sidsector9
Copy link
Member Author

What's done until now:

  1. Extract username/repository_name from the URL
  2. Obtain package_name from composer.json file using raw GitHub URL
  3. If this package_name is identical to the username/repository_name, then go with the normal flow, if not, then instead of username/repository_name, require will now refer to package_name and inside repositories, package_name will map to the repository URL.

.wp-cli/packages/composer.json will have a structure as

{
    "require": {
        "package_name": "dev-master"
    },
    "repositories": {
        "package_name": {
            "type": "vcs",
            "url": "github.com:user/repository_name.git"
        }
    }
}

@schlessera
Copy link
Member

We need to add a test command that include the above problem to the wp-cli-test org and reference that package inside of the feature test instead. I'll create a package now.

@danielbachhuber
Copy link
Member

@schlessera @Sidsector9 Should this land in the next release, or does it need more work?

@Sidsector9
Copy link
Member Author

@danielbachhuber This is ready.

@schlessera schlessera added this to the 1.0.5 milestone Oct 2, 2017
@schlessera schlessera dismissed danielbachhuber’s stale review October 2, 2017 07:17

Required test was added.

@schlessera schlessera merged commit 49f394e into wp-cli:master Oct 2, 2017
@schlessera schlessera changed the title GH#4232 Added check for package name Support package names that differ from repository names Oct 2, 2017
schlessera added a commit that referenced this pull request Jan 5, 2022
GH#4232 Added check for package name
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants