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

Commit 3efed220 breaks existing ScriptHandlers #3898

Closed
uwej711 opened this issue Apr 1, 2015 · 12 comments
Closed

Commit 3efed220 breaks existing ScriptHandlers #3898

uwej711 opened this issue Apr 1, 2015 · 12 comments

Comments

@uwej711
Copy link

uwej711 commented Apr 1, 2015

The mentioned commit changes the event from CommandEvent to Event for ScriptHandlers, which breaks my own handlers and also those of SensioDistributionBundle. You get this if you do a simple composer self-update.

Is this intentionally?

@stevelacey
Copy link

Just needs to be in a message not a title, boom 3efed22

@alcohol
Copy link
Member

alcohol commented Apr 9, 2015

@uwej711
Copy link
Author

uwej711 commented Apr 9, 2015

Anyway, this makes it impossible to use composer with older ScriptHandlers

@alcohol
Copy link
Member

alcohol commented Apr 9, 2015

That's ok, composer is still an alpha, incompatible changes will happen. Upgrade your scripthandlers.

I don't entirely see the problem by the way, CommandEvent extends from Event, so both are still ok.

Also, for BC: https://github.com/composer/composer/blob/master/src/Composer/EventDispatcher/EventDispatcher.php#L222

So what exactly is broken now for you? Can you actually provide a reproducible scenario, other than some comment without context?

@Seldaek
Copy link
Member

Seldaek commented Apr 9, 2015

I think it only breaks if you had incorrect (too loose?) type hints. For most people the BC layer worked, in a few cases yes there is breakage and I am sorry but I think updating and releasing a new version of those few script handlers is probably not so hard. On the other hand we had a huge mess here so it had to be cleaned up.

@uwej711
Copy link
Author

uwej711 commented Apr 9, 2015

Im affected as a user of SensioDistributionBundle. Will there be an update?

@alcohol
Copy link
Member

alcohol commented Apr 9, 2015

Check with them?

@uwej711
Copy link
Author

uwej711 commented Apr 9, 2015

I conclude this was intentionally and there is currently no way to resolve this other than stopping updating composer for a while.

@uwej711 uwej711 closed this as completed Apr 9, 2015
@alcohol
Copy link
Member

alcohol commented Apr 9, 2015

Because asking the maintainer of the broken package to fix it is not an option..? Ok..

@uwej711
Copy link
Author

uwej711 commented Apr 9, 2015

Well no, that is not the problem. But as you said, it needs to be fixed there. So closing this.

@alcohol
Copy link
Member

alcohol commented Apr 9, 2015

Though https://github.com/sensiolabs/SensioDistributionBundle/blob/master/Composer/ScriptHandler.php to me looks to be using proper typehints, so the BC layer provides for that. I see no reason why SensioDistributionBundle would not work.

@uwej711
Copy link
Author

uwej711 commented Apr 9, 2015

@alcohol you are right, thanks for pointing me to the right direction. The mistake was in our own code not using type hints for the events at all. Please accept my apologies.

Uwe

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

No branches or pull requests

4 participants