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

I'm Confused By Some of the Directions #530

Open
1 task done
tomonroad opened this issue Nov 14, 2023 · 5 comments
Open
1 task done

I'm Confused By Some of the Directions #530

tomonroad opened this issue Nov 14, 2023 · 5 comments
Labels
documentation Solely about the documentation of the project.

Comments

@tomonroad
Copy link

Verification

  • I have searched existing issues and discussions for my problem.

Which file do you have feedback about? (leave blank if not applicable)

docs/usage/recommended-workflows.md

Feedback

I don't understand what Step 3 in the Remove a Patch instructions means.

I don't know why you need to do Step 4. Is this for when you have a patch that requires other patches in dependencies of the patched module to work?

@tomonroad tomonroad added the documentation Solely about the documentation of the project. label Nov 14, 2023
@cweagans
Copy link
Owner

Step 3: the intent is that you currently have a patched dependency (say symfony/process). If you remove the patch that is defined for symfony/process, you need to go rm -r vendor/symfony/process and then re-install symfony/process to get the unpatched version of that dependency (which is what Step 4 accomplishes).

Is there a way that this could have been written that would have made more sense to you?

@GamesmenJordan
Copy link

I agree that the instructions are unclear.

Manually delete the dependency that you removed a patch from (the location of the dependency will vary by project, but a good starting point is to look in the vendor/ directory).

Run composer patches-repatch to delete patched dependencies and reinstall them with any defined patches

On the surface it looks like step 4 is deleting all patched dependencies (including the one we just deleted the patch definition for).

If we skip step 3, what happens? If it has other patch definitions, do they still get applied? If it has no patch definitions left but we didn't delete the files, do the old patches remain in place?

If yes to both, then Step 4 should probably read something like:

Run composer patches-repatch to install the deleted dependencies and reinstall any defined patches to all dependencies

Somewhat relatedly, this behavior seems like a regression from 1.x which handled this more or less automatically.

@RobinHoutevelts
Copy link

Somewhat relatedly, this behavior seems like a regression from 1.x which handled this more or less automatically.

It's a different workflow. Not really a regression because that implies a bug.

You are right in 1.x this worked better out of the box. If I understand correctly it also caused subtle bugs which were hard/impossible to fix.

That's why this new workflow got introduced that uses the patches.lock.json and patches-relock/patches-repatch commands.

On the surface it looks like step 4 is deleting all patched dependencies (including the one we just deleted the patch definition for).

Step 4 (patches-repatch) repatches (after removing and reinstalling) all *remaining* patched dependencies.
It will only remove "the one we just deleted the patch definition for" if there are other patch definitions left for it.

If we skip step 3, what happens? If it has other patch definitions, do they still get applied?

Yes, if there are other patch definitions left, they will be applied when you run patches-repatch.
The patch you removed won't be reapplied again because you ran patches-relock in step 2.

If it has no patch definitions left but we didn't delete the files, do the old patches remain in place?

When there are no patch definitions left, the patches-repatch command won't have reinstalled your dependency.
In that case the dependency stays untouched and the patches indeed remain in place.
That's why it's documented to remove it yourself.


These are the steps I take when I want to remove a patch for my-org/package.

  • Remove the patch from my composer.json
  • Run composer patches-relock to update the patches.lock.json
  • Verify the patch I removed is no longer in the patches.lock.json file
  • composer update --lock to update my composer.lock file
  • Remove the package manually so it will be installed again
    • rm -rf vendor/my-org/package
  • composer install
  • Commit composer.json, composer.lock, and patches.lock.json.

@GamesmenJordan
Copy link

I meant regression in a more colloquial term, as in "worse than before".

You are right in 1.x this worked better out of the box. If I understand correctly it also caused subtle bugs which were hard/impossible to fix.

Not sure I buy this argument, if the steps can be done manually, they can be done automatically too. I built scripts on top of 1.x to generate patches from a directory and apply them, if I need to regenerate the patches it reinstalls the unpatched module, creates the patch, then reapplies the patch. Not that much different to what is going on here except that I don't need to manually delete anything.

So far I'm really not a fan of the changes with 2.x workflow, it's added more complexity for no benefit.

@cweagans
Copy link
Owner

It was extraordinarily complex in 1.x. Probably one of the biggest sources of bugs. There are lots of edge cases wrt installing/updating a single package (vs your entire project). It was a constant headache to support.

You're welcome to script this in your own project, but it'll never be a one-size-fits-all sort of thing, so this probably won't be done in the main plugin. If anything, the more explicitly defined workflows make that possible.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Solely about the documentation of the project.
Projects
None yet
Development

No branches or pull requests

4 participants