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

Patched projects will be deleted and re-downloaded and patched #42

Open
piepkrak opened this issue Apr 20, 2016 · 27 comments

Comments

@piepkrak
Copy link

commented Apr 20, 2016

This causes issues when drupal/drupal (version 7) is patched as everything resides in whatever location Drupal has been installed in.

As far as I know it is not possible to run Drupal itself as a dependency, so we would need something like https://github.com/winmillwill/drupal-tangler to resolve this issue. Do you have any other working solutions to this issue?

@morvans

This comment has been minimized.

Copy link

commented Apr 21, 2016

Isn't something like derhasi/composer-preserve-paths designed to prevent such deletions ?
Hint: tested it but, doesn't work in my case (drupal7 too), maybe composer-patches needs to integrate with it.

{
  "name": "madeinlune/drupal7-sandbox",
  "description": "Drupal 7 Composer Sandbox",
  "repositories": [{
    "type": "composer",
    "url": "https://packagist.drupal-composer.org"
  }],
  "require": {
    "composer/installers": "~1.0",
    "cweagans/composer-patches": "1.4.0",
    "derhasi/composer-preserve-paths": "0.1.*",
    "drupal/drupal": "7.43.0",
    "drupal/admin_menu": "7.3.0-rc5"
  },
  "conflict": {
    "drupal/core": "8.*"
  },
  "scripts": {
    "post-create-project-cmd": [
      "rm README.md LICENSE .travis.yml phpunit.xml.dist"
    ]
  },
  "config": {
    "vendor-dir": "vendor"
  },
  "minimum-stability": "dev",
  "prefer-stable": true,
  "extra": {
    "installer-paths": {
      "www/": ["type:drupal-core"],
      "www/sites/all/modules/contrib/{$name}/": ["type:drupal-module"],
      "www/sites/all/themes/contrib/{$name}/": ["type:drupal-theme"],
      "www/sites/all/libraries/{$name}/": ["type:drupal-library"],
      "www/sites/all/drush/{$name}/": ["type:drupal-drush"],
      "www/profiles/{$name}/": ["type:drupal-profile"]
    },
    "patches": {
      "drupal/drupal": {
        "#728702": "https://drupal.org/files/issues/install-redirect-on-empty-database-728702-36.patch",
        "#1470656": "https://drupal.org/files/drupal-1470656-14.patch",
        "#865536": "https://drupal.org/files/drupal-865536-204.patch",
        "#1772316": "https://drupal.org/files/drupal-7.x-allow_profile_change_sys_req-1772316-21.patch",
        "#1275902": "https://drupal.org/files/1275902-15-entity_uri_callback-D7.patch",
        "#1162752": "https://drupal.org/files/drupal-add_taxonomy_filter_to_content_admin-1162752-14.patch"
      }
    },
    "preserve-paths": [
      "www/sites/all/modules",
      "www/sites/all/themes",
      "www/sites/all/libraries",
      "www/sites/all/drush",
      "www/sites/default"
    ]
  }
}

@cweagans cweagans added the bug label Apr 21, 2016

@cweagans

This comment has been minimized.

Copy link
Owner

commented Apr 21, 2016

Hm, composer-preserve-paths should take care of this, but evidently, it's not. I'll take a look.

@grahl

This comment has been minimized.

Copy link

commented Apr 25, 2016

I'm also encountering this issue, downgrading to 1.3.0 of composer-patches doesn't help, neither does explicitly declaring the core root path under preserve-paths.

@cweagans

This comment has been minimized.

Copy link
Owner

commented Apr 25, 2016

Was this ever working (in any version of this plugin)? It's possible that it had never worked.

@morvans

This comment has been minimized.

Copy link

commented Apr 25, 2016

I don't know for sure. I encountered this behavior recently when I tried to use the plugin on a D7 project for the first time. I was only using it on D8 previously.
It may have never work with a dependency on D7.

Le 25 avril 2016 18:45:16 CEST, Cameron Eagans notifications@github.com a écrit :

Was this ever working (in any version of this plugin)? It's possible
that it had never worked.


You are receiving this because you commented.
Reply to this email directly or view it on GitHub:
#42 (comment)

Envoyé de mon téléphone Android avec K-9 Mail. Excusez la brièveté.

@piepkrak

This comment has been minimized.

Copy link
Author

commented Apr 25, 2016

It seems none of the events composer-preserve-paths is subscribed to trigger when composer-patches does $installationManager->uninstall on the patched package. Even though one could expect that ScriptEvents::PRE_PACKAGE_UNINSTALL would take care of this.

I have seen some deprecation notices in Composer/Composer regarding these events. Wonder if that has anything to do with it.
https://github.com/composer/composer/blob/e2056499cb9641df98c7b1c37279a6085394808a/src/Composer/Script/ScriptEvents.php#L182

I will follow up next weekend.

@shahinam

This comment has been minimized.

Copy link

commented Apr 29, 2016

Here is the link to issue - drupal-composer/preserve-paths#10

@piepkrak

This comment has been minimized.

Copy link
Author

commented May 10, 2016

I think this use-case was never anticipated. Shouldn't the event dispatcher be called before $installationManager->uninstall($localRepository, $uninstallOperation);?

My hypothesis is composer-preserve-paths will back everything up when PRE_PACKAGE_UNINSTALL fires before the uninstall operation in this plugin. Then when the package is installed again, it will be patched because of the implicit trigger of POST_PACKAGE_INSTALL. Also composer-preserve-paths will restore the configured preserved paths.

Does anyone have any usefull feedback on this before diving into the IDE?

@cweagans

This comment has been minimized.

Copy link
Owner

commented May 10, 2016

Just clicking through some of the Composer files on Github -- I didn't see anywhere that PRE_PACKAGE_UNINSTALL would have been dispatched. It's entirely possible that simply dispatching that event before $installationManager->uninstall() could do the trick.

@cweagans

This comment has been minimized.

Copy link
Owner

commented May 10, 2016

(And for the record, I was aware of composer-preserve-paths -- I just figured that calling $installationManager->uninstall() would have triggered the event. I don't actually use composer-preserve-paths, though, and therefore didn't test that it played nicely with composer-patches. Happy to maintain that support if you open a PR for it)

@piepkrak

This comment has been minimized.

Copy link
Author

commented May 10, 2016

I understand. Would make sense from a semantic standpoint that $installationManager->uninstall() would fire events implicitly. No harm intended in my feedback. ;-)

I use composer-preserve-paths for Drupal 7 as 3rd party packages are stored inside the root package. I don't like this, but it seems necessary if you don't want to symlink your packages into the root package.

I am going to look into this and create a PR if I can get it to work. This would not only benifit composer-preserve-paths, but all composer plugins which hook in to the event bus. I did see the event dispatcher needs a lot of arguments, so no quick fix today.

@jurgenhaas

This comment has been minimized.

Copy link

commented Jul 22, 2016

This might be a duplicate of #26 and I've submitted a PR there as well.

@eelkeblok

This comment has been minimized.

Copy link

commented Oct 25, 2016

@piepkrak Have you made any progress with this? Looking into the problem, I basically came to the same conclusion (it's funny how an issues can look like total gibberish until you've actually done some of the troubleshooting yourself, after which it makes perfect sense :)). I was not quite sure how to fill the various inputs the event dispatcher needs to dispatch a package event, though.

@piepkrak

This comment has been minimized.

Copy link
Author

commented Oct 25, 2016

Unfortunately not. Dispatching the event was also the blocker for me at the time. Will try to free up some time next week.

@SebCorbin

This comment has been minimized.

Copy link

commented Mar 14, 2017

Related question composer/composer#6261

@SebCorbin

This comment has been minimized.

Copy link

commented Mar 22, 2017

PRE_PACKAGE_UNINSTALL is dispatched at https://github.com/composer/composer/blob/122e422682d961233cc5db8b2102cd98b049d4f9/src/Composer/Installer.php#L572

I tried dispatching the event but it is very ugly, but usable, see SebCorbin@1cb9bac

@cweagans

This comment has been minimized.

Copy link
Owner

commented Mar 22, 2017

Is there something that we can do in this plugin (other than manually dispatching the event) that would cause that line to be run? I haven't stepped through this too much, as this has been fairly low priority for me.

@SebCorbin

This comment has been minimized.

Copy link

commented Mar 22, 2017

For the moment, no: either we ask the composer team to dispatch the event on uninstall() (but that would mean on each installation managers or we stick to dispatching the event ourselves.

@clemens-tolboom

This comment has been minimized.

Copy link

commented Nov 2, 2017

Why not add a PR from SebCorbin@1cb9bac for the time being. We still face this with Drupal 7 projects on core patched and upgrade/downgrades of core.

@jcnventura

This comment has been minimized.

Copy link

commented Jan 19, 2018

I've asked @SebCorbin to create a PR of that commit into this project. I think it makes sense to explicitly fire those events in composer-patches, if composer doesn't do that implicitly.

@jcnventura

This comment has been minimized.

Copy link

commented Jan 19, 2018

For now, added the fix to the 7.x branch of https://github.com/drupal-composer/drupal-project/tree/7.x

@jcnventura

This comment has been minimized.

Copy link

commented Mar 19, 2018

@cweagans It seems that @SebCorbin is not going to create the PR. Would you consider merging SebCorbin@1cb9bac anyway?

@cweagans

This comment has been minimized.

Copy link
Owner

commented Mar 19, 2018

I'm not sure. I don't really want to just blindly merge it because even though it solves the problems with composer-preserve-paths, it might have unintended effects on other plugins that are listening for those events because that change dispatches the events with empty data. I don't know that we can do much of anything else, but I think it's worth some investigation before merging at least. To my knowledge, that investigation has not been done.

@codebymikey

This comment has been minimized.

Copy link

commented Mar 1, 2019

codebymikey added a commit to zodiacmedia/composer-patches that referenced this issue Mar 1, 2019
Add pre and post uninstall hooks when a patch is being applied.
Replicating the `\Composer\Installer::doInstall` logic.

Refs cweagans#42
codebymikey added a commit to zodiacmedia/composer-patches that referenced this issue Mar 1, 2019
Add pre and post uninstall hooks when a patch is being applied.
Replicating the `\Composer\Installer::doInstall` logic.

Refs cweagans#42
@jcnventura

This comment has been minimized.

Copy link

commented Mar 12, 2019

@cweagans Would there be a way to force composer-patches to patch itself in a post-create-project-cmd step?

Or would you be willing to add a pre-2.x patch if we modify the code in this commit to only be active if we set a flag on the composer.json config?

@jcnventura

This comment has been minimized.

Copy link

commented Mar 12, 2019

Also note that if composer-exit-on-patch-failure is set true, the code should try to cleanup before exiting by calling the POST_PACKAGE_UNINSTALL event.

That way stuff doesn't get stuck in the preserve-paths limbo at ~/.cache/composer/preserve-paths.

jcnventura pushed a commit to jcnventura/composer-patches that referenced this issue Mar 12, 2019
@jcnventura

This comment has been minimized.

Copy link

commented Mar 12, 2019

Created PR #257

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
You can’t perform that action at this time.