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

exclude_files #83

Closed

Conversation

BrianHenryIE
Copy link
Contributor

@BrianHenryIE BrianHenryIE commented Nov 23, 2020

Add a config option to exclude files based on regex match.

Problem encountered because illuminate/collections's namespace is Illuminate\Support!

{
    "name": "illuminate/collections",
...
    "autoload": {
        "psr-4": {
            "Illuminate\\Support\\": ""
        },
        "files": [
            "helpers.php"
        ]
    },

When I try to use both it and actual illuminate/support I get errors because Mozart is using the namespace to choose the target folder, not the package name.

File already exists at path: src/vendor/Illuminate/Support/LICENSE.md  
File already exists at path: src/vendor/Illuminate/Support/composer.json  

So I wrote some code to exclude them but then got:

File already exists at path: src/vendor/Illuminate/Support/helpers.php

The two helpers.php are not the same. But they're not being PSR-4 auto-loaded anyway, so I excluded it and Mozart does not complain.

Broken:

{
  "name": "brianhenryie/mozart-illuminate-file-clash",
  "require": {
    "illuminate/support": "^8.0",
    "illuminate/collections": "^8.0"
  },
  "require-dev": {
    "coenjacobs/mozart": "dev-master"
  },
  "extra": {
    "mozart": {
      "dep_namespace": "BH\\",
      "dep_directory": "/src/vendor/",
      "classmap_directory": "/src/classes/",
      "classmap_prefix": "BH_",
      "delete_vendor_directories": false
    }
  },
  "scripts": {
    "post-install-cmd": [
      "vendor/bin/mozart compose"
    ],
    "post-update-cmd": [
      "vendor/bin/mozart compose"
    ]
  }
}

working:

{
  "name": "brianhenryie/mozart-illuminate-file-clash",
  "require": {
    "illuminate/support": "^8.0",
    "illuminate/collections": "^8.0"
  },
  "require-dev": {
    "coenjacobs/mozart": "dev-master",
    "cweagans/composer-patches": "~1.0"
  },
  "extra": {
    "patches": {
      "coenjacobs/mozart": {
        "Exclude Files": "https://github.com/coenjacobs/mozart/pull/83.patch"
      }
    },
    "mozart": {
      "dep_namespace": "BH\\",
      "dep_directory": "/src/vendor/",
      "classmap_directory": "/src/classes/",
      "classmap_prefix": "BH_",
      "delete_vendor_directories": false,
      "exclude_files": ["/\\.md$/","/composer\\.json/","/helpers\\.php/"]
    }
  },
  "scripts": {
    "post-install-cmd": [
      "vendor/bin/mozart compose"
    ],
    "post-update-cmd": [
      "vendor/bin/mozart compose"
    ]
  }
}

The name `exclude_files` could be better -> `exclude_files_from_copy` to be distinct from potentially `exclude_files_from_replace` or any confusion with the `files` autoloader.
@coenjacobs
Copy link
Owner

Not sure how I feel about this, for now (hint: this is a pointer to start convincing me 😉 ).

I get why this is useful, but it feels like more than a monkey patch to a specific problem than Mozart is a monkey patch in itself already. No doubt there are a ton more of these weird setups out there in the wild that may or may not require a specific handling in Mozart. What this does in its current state, is open the door for even weirder bugs when the regex applies to files in multiple packages, even the ones the author didn't intend to target.

What I initially think (and just sharing this here, now it's on top of mind, this is not necessarily the way to do it) is better, is to have a specific piece of configuration per package, so we can applies these kind of exclusion rules specific to that package. Now that I type this, it also sounds like a bit overkill as well, but I'm just sharing my general idea here.

@BrianHenryIE
Copy link
Contributor Author

BrianHenryIE commented Nov 23, 2020

In order to allow targeting specific packages, I made sure to use the full path from $file->getRealPath()and not just the filename.

But yeah, it's a bit of a hacky way to fix problems with PSR-0/classmaps that might be fixed better if I took the time to learn the intricacies. I'm happy for the PR to sit here to be used with composer-patches until the problem is thrashed out.

One likely change is to exclude files in the files autoloader key from being copied during a Classmap or PSR-4 move operation.

Looking at the illuminate packages, the problem could be solved by also only copying .php files. The issue is that the sources files are in the project root, so Mozart copies the licence (prompting #87) and composer.json and failing when the second package is being processed.

@coenjacobs
Copy link
Owner

I made sure to use the full path from $file->getRealPath()and not just the filename.

Yes, but what if my package (used in the same project, by a separate person) uses a legitimate helpers.php file, which I do want to copy? What I mean is that there is no way to have your regex target a specific package, for it to apply on. It basically applies to any and all packages that are processed by Mozart, which might lead to unexpected behaviour.

@BrianHenryIE
Copy link
Contributor Author

You can use the package name in the regex:"/illuminate\\/support\\/helpers\\.php/".

@BrianHenryIE BrianHenryIE marked this pull request as draft December 14, 2020 05:47
@BrianHenryIE BrianHenryIE deleted the file-exclusion-patterns branch May 3, 2021 02:22
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 this pull request may close these issues.

None yet

2 participants