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

Add explicit possibility to selectively replace vendor classes #4732

Closed
malkusch opened this Issue Dec 24, 2015 · 8 comments

Comments

Projects
None yet
5 participants
@malkusch

malkusch commented Dec 24, 2015

Sometimes an upstream class in vendor is not extensible enough with conventional language constructs (e.g. it's final, has no suitable extension point or simply an existing bug won't be fixed in that version stream anymore). In such case only replacing that offending class with an own implementation is viable.

Currently this is possible because composer's class loader prefers PSR-4 over PSR-0. So if the vendor package uses PSR-0 you could simply provide a replacement by adding a PSR-4 path to the new class. However this is an implicit implementation detail of composer which is only available by reading the code of the private method in ClassLoader::findFileWithExtension() and could change with any release.

Composer as the central authority for loading classes brings the great opportunity to replace selectively vendor classes completely. Let's specify an explicit mechanism to define a sequence of class paths.

The detailed specification should come with this discussion. For a starter we could discuss a simple overwrite field to the composer.json schema which would accept the same fields as autoload but has a higher priority in the autoloading process. Or we can target an extensible and well defined class loader hierarchy as we have in Java.

@alcohol

This comment has been minimized.

Show comment
Hide comment
@alcohol

alcohol Dec 26, 2015

Member

You've never heard of interfaces? Wrappers? Adapters?

There is zero need to selectively replace an existing class.

Member

alcohol commented Dec 26, 2015

You've never heard of interfaces? Wrappers? Adapters?

There is zero need to selectively replace an existing class.

@alcohol alcohol added the Question label Dec 26, 2015

@malkusch

This comment has been minimized.

Show comment
Hide comment
@malkusch

malkusch Dec 27, 2015

There is zero need to selectively replace an existing class.

I can understand your reluctance and I would also not promote that feature highly. However I disagree with zero need. Imagine you would depend on a third party library which you want to behave slightly differently (e.g. you want to log a message). Sometimes the author didn't provide extension points which would enable this. All your friendly questions will not help in this case, as the offending class might be final or is instantiated as an implementation detail.

Forking and patching the class in the fork is a viable option, but then again why do I need to fork in the first place, if technically replacing the offending class is possible?

malkusch commented Dec 27, 2015

There is zero need to selectively replace an existing class.

I can understand your reluctance and I would also not promote that feature highly. However I disagree with zero need. Imagine you would depend on a third party library which you want to behave slightly differently (e.g. you want to log a message). Sometimes the author didn't provide extension points which would enable this. All your friendly questions will not help in this case, as the offending class might be final or is instantiated as an implementation detail.

Forking and patching the class in the fork is a viable option, but then again why do I need to fork in the first place, if technically replacing the offending class is possible?

@staabm

This comment has been minimized.

Show comment
Hide comment
@staabm

staabm Dec 27, 2015

Contributor

@malkusch https://github.com/krakjoe/uopz is what you are after.

Allow this kind of monkey patching at composer level sounds like a can of worms to me, which should better stay closed.

Contributor

staabm commented Dec 27, 2015

@malkusch https://github.com/krakjoe/uopz is what you are after.

Allow this kind of monkey patching at composer level sounds like a can of worms to me, which should better stay closed.

@alcohol

This comment has been minimized.

Show comment
Hide comment
@alcohol

alcohol Dec 28, 2015

Member

@malkusch

Sometimes the author didn't provide extension points which would enable this.

There is always a way to do this.

Member

alcohol commented Dec 28, 2015

@malkusch

Sometimes the author didn't provide extension points which would enable this.

There is always a way to do this.

@malkusch

This comment has been minimized.

Show comment
Hide comment
@malkusch

malkusch Dec 28, 2015

uopz is what you are after.

Thank you for that suggestion. I'm aware of PHP extensions which would do the job (e.g. runkit to name a further one). This proposal is specifically targeted to use plain PHP's autoload mechanism for most portability.

Well, I can see that until now there's no support for this idea. I'm really Sorry for not coming up with any new use cases, but elaborating on the existing ones:

Mocks for final classes

Surprisingly I see final very rarely in PHP land. So it might happen that it never occurred to you. But it is a very valid decision to design classes explicitly without inheritance in mind. Now consider the case that you write a class which depends on such a third party final class. You could now simply instantiate that class and wire up all it's dependencies. But depending on the complexity of the object graph, the intention of your test might get easily lost in a jungle of instantiation code.

No suitable extension point

This is actually more a generalization of the final argument. Final is not the only way to prevent extension. Consider a class which creates some helper classes with new. This is absolutely valid as clients shouldn't care about such implementation details. Eventually in reality it turns out that we specifically need logging somewhere in such an instantiated class. As we don't control instantiation we cannot inject a modified derivation.

Won't be fixed

Let me present my current use case: I'm the author of php-mock which is a mocking tool targeted at PHP's built-in functions. As I don't want to pollute the cognitive load of developers I do offer integrations for well known existing frameworks. One of them will be php-mock-prophecy. However there's one offending line of code in upstream which prevents me from implementing prophecies for built-ins which use pass-by-reference (e.g. exec()). This can only be fixed in upstream, however the current discussion reveals that upstream considers prophecies for pass-by-reference as a design smell and won't support that. This is a very valid opinion for their use case. But as I'm targeting prophecies for PHP built-ins the existence of pass-by-reference is a hard fact. A very experimental monkey patch enabled me to provide the full functionality.

malkusch commented Dec 28, 2015

uopz is what you are after.

Thank you for that suggestion. I'm aware of PHP extensions which would do the job (e.g. runkit to name a further one). This proposal is specifically targeted to use plain PHP's autoload mechanism for most portability.

Well, I can see that until now there's no support for this idea. I'm really Sorry for not coming up with any new use cases, but elaborating on the existing ones:

Mocks for final classes

Surprisingly I see final very rarely in PHP land. So it might happen that it never occurred to you. But it is a very valid decision to design classes explicitly without inheritance in mind. Now consider the case that you write a class which depends on such a third party final class. You could now simply instantiate that class and wire up all it's dependencies. But depending on the complexity of the object graph, the intention of your test might get easily lost in a jungle of instantiation code.

No suitable extension point

This is actually more a generalization of the final argument. Final is not the only way to prevent extension. Consider a class which creates some helper classes with new. This is absolutely valid as clients shouldn't care about such implementation details. Eventually in reality it turns out that we specifically need logging somewhere in such an instantiated class. As we don't control instantiation we cannot inject a modified derivation.

Won't be fixed

Let me present my current use case: I'm the author of php-mock which is a mocking tool targeted at PHP's built-in functions. As I don't want to pollute the cognitive load of developers I do offer integrations for well known existing frameworks. One of them will be php-mock-prophecy. However there's one offending line of code in upstream which prevents me from implementing prophecies for built-ins which use pass-by-reference (e.g. exec()). This can only be fixed in upstream, however the current discussion reveals that upstream considers prophecies for pass-by-reference as a design smell and won't support that. This is a very valid opinion for their use case. But as I'm targeting prophecies for PHP built-ins the existence of pass-by-reference is a hard fact. A very experimental monkey patch enabled me to provide the full functionality.

@alcohol

This comment has been minimized.

Show comment
Hide comment
@alcohol

alcohol Dec 28, 2015

Member

Well, technically I guess this can already be done. If Composer detects a duplicate class (namespace+classname), it will only load one of the available choices. If you simply make sure your class is detected before the one from the other library, you should be good to go. Not sure how you can make sure of that though... :d

Member

alcohol commented Dec 28, 2015

Well, technically I guess this can already be done. If Composer detects a duplicate class (namespace+classname), it will only load one of the available choices. If you simply make sure your class is detected before the one from the other library, you should be good to go. Not sure how you can make sure of that though... :d

@hackel

This comment has been minimized.

Show comment
Hide comment
@hackel

hackel Jan 8, 2016

I've been trying to make this work as well. I thought that Composer loaded dependencies in order: classpath, psr-0, psr-4, but recently (with a composer update?) I started getting "Ambiguous class resolution" warnings, and the vendor class (psr-4) was getting loaded instead of my class (defined in classpath). I think we just need some consistency here.

My particular use-case is that the vendor library I'm using calls "new Someclass" instead of injecting it, and that class has a critical error. I would have to overload so many different classes in the library to get it to work, that I might as well just maintain my own fork. I realise this isn't the best approach, but sometimes we just need to move faster than upstream and maintaining a ton of independent forks is an unnecessary nuisance when it's something this simple.

Edit: I was able to solve my particular case by excluding the vendor class, adding it to autoload.exclude-from-class-map.

hackel commented Jan 8, 2016

I've been trying to make this work as well. I thought that Composer loaded dependencies in order: classpath, psr-0, psr-4, but recently (with a composer update?) I started getting "Ambiguous class resolution" warnings, and the vendor class (psr-4) was getting loaded instead of my class (defined in classpath). I think we just need some consistency here.

My particular use-case is that the vendor library I'm using calls "new Someclass" instead of injecting it, and that class has a critical error. I would have to overload so many different classes in the library to get it to work, that I might as well just maintain my own fork. I realise this isn't the best approach, but sometimes we just need to move faster than upstream and maintaining a ton of independent forks is an unnecessary nuisance when it's something this simple.

Edit: I was able to solve my particular case by excluding the vendor class, adding it to autoload.exclude-from-class-map.

@Seldaek

This comment has been minimized.

Show comment
Hide comment
@Seldaek

Seldaek Feb 21, 2016

Member

AFAIK, if your package defines a psr-0 or psr-4 autoload rule that is either more specific or just clashes with a dependency's rule, the root package's rule will be loaded first and hence you can override classes easily that way.

Member

Seldaek commented Feb 21, 2016

AFAIK, if your package defines a psr-0 or psr-4 autoload rule that is either more specific or just clashes with a dependency's rule, the root package's rule will be loaded first and hence you can override classes easily that way.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment