Problem with Stripe ILMerged into PaymentProviders
I just went through the process of upgrading from TC 3.2.4 to 3.3.1 and everything seems to work, except for one issue. Our custom Stripe based payment provider uses a newer version of Stripe.net than what's ILMerged into the TeaCommerce.PaymentProviders.dll, and when we add the newer Stripe.net assembly to the site we end up with the same types/classes in the same namespace (Stripe) coming from two different assemblies, and if we use a Stripe identifier anywhere we get compile errors.
"The type 'PaymentIntent' exists in both 'Stripe.net, Version=27.18.0.0, Culture=neutral, PublicKeyToken=null' and 'TeaCommerce.PaymentProviders, Version=3.1.7114.13982, Culture=neutral, PublicKeyToken=null'"
I first thought that we could work around this by using assembly aliasing, but that feature seems to be unavailable with web site projects (and is not ideal anyway). So then maybe the only solution is to recompile the PaymentProviders assembly locally using the correct Stripe assembly? If yes, what is the recommented (easiest) approach to doing this?
ILMerging one third-party assembly into a second third-party assembly might be a less than perfect practice, particulary in Stripe.net's case since it's being updated fairly frequently (far more frequently than TeaCommerce). Thoughts?
I agree, it's less than ideal. The problem we have with the current packaging system is that there is just no way to manage versions of third party dependencies so if I just include the DLL in my package it'll replace your's downgrading you to an older version. But if we don't include it, then the payment provider won't work without it. We are kind of damned if we do and damned if we don't.
That said, I thought ILMerging was meant to prevent these issue (or maybe I needed to internalize them but I'm sure I hit problems doing that). Best I can suggest for now if you do need to use a newer Stripe.NET DLL is to grab the source from github and compile it manually, not merging the DLLs.
In TC for v8 we'll have nuget packages and so we can have nuget dependencies, but what to do about Umbraco package files is still an issue. I'm open to suggestions though?
PS I have recently switched to ILRepack instead of ILMerge because someone was having an issue with the ILMerged DLL, but ILRepack worked fine. I wonder if internalizing with ILRepack will have the same issues as with ILMerge. I'll have to do some more testing.
Internalizing the Stripe assembly might work, but there's still code smell. I (the user) would have to remember that the standard PaymentProviders assembly is using one version of Stripe, while my code is using another.
I'm not an expert on project configurations, but would it be possible to leave the Stripe dll out of the ILMerged assembly, and instead require me (your user) to supply it (Nuget or otherwise)? If yes, maybe it also would be possible for you to specify (in the project config) minimum/maximum versions of Stripe that are compatible with PaymentProviders?
The problem if we were to not include the Stripe.NET DLL is that now Tea Commerce installs incomplete and as it currently stands, it installs everything and all providers when you install the package. If we don't deploy the Stripe.NET DLL , then it will either YSOD immediately (not a great experience) or just won't show Stripe in the providers list (which as all others with no dependency will be in the list, devs will just assume that Stripe isn't supported, so equally not a great experience).
I appreciate that for people working with Stripe, the ILMerge is a problem, but really, devs including a new Stripe.NET DLL are likely more clued up and able to work around this, than folks who just want to install it and it works.
IIRC it is possible to embed a DLL into another DLL and then you can implement a sort of finder so I think it will look in the AppDomain but if it's not found, ask these finders. So it might be that we have to go down that route and embed it that way. If this is correct, then you could drop a newer DLL in the bin folder and if it sees it, it will use that, but only fallback to the embedded version if it's not found. Of course, this still poses the risk that the payment provider relies on the older code base and if there are breaking changes in the newer DLL, then this will break the provider.
PS In v8, I'm thinking that all providers will be their own package / nuget package and you just install what you need. With that, I think it's more reasonable to exclude a dependency if we must, or better, provider some UI during the install to check / install if not found.
Also, regarding "I (the user) would have to remember that the standard PaymentProviders assembly is using one version of Stripe, while my code is using another." this would potentially be the case regardless as it might be that the API could have changed so really, we don't want you upgrading the DLL the provider uses as it could break the provider.
Really, I think if you want to use a specific version of Stripe.NET, the safest route is to fork the provider and create a custom implementation of it, all compiled against the same version.
In the payment providers repo on github, I have actually broken all the providers up into their own projects so that this is easier to do.
"...we don't want you upgrading the DLL the provider uses as it could break the provider." This is why I thought maybe you could specify in the config which versions the provider code is compatible with.
"...I think if you want to use a specific version of Stripe.NET, the safest route is to fork the provider and create a custom implementation of it, all compiled against the same version."
This is what we have done :-) And inside our provider/assembly everything is fine. But we had a need to reference Stripe in the web site project and that's where the conflict occurred. EDIT: not against the same version, it's our own assembly, side by side with the standard providers assembly.
I'm just giving feedback and trying to be constructive. I can easily solve the issues for us by recompiling the providers assembly, so things are good in that respect. But it will be a thing for us to remember for time there's a Tea Commerce upgrade.
Minor side note, the Tea Commerce upgrade/install did in fact YSOD due to the Stripe conflicts. I was waiting on the installation page forever until I gave up and started debugging. That's when I noticed the compile errors. Hmm, does that mean the YSOD will happen on the next upgrade as well? (until we replace the providers dll with our recompiled version again)?
We are very happy that Tea Commerce now is under active development, and we will be upgrading to Umbraco / TC 8 when the time comes. Keep it up!
Oh, I'm not dismissing your feedback by any means, it's completely valid and useful to know the issues people are facing. I don't want anyone to have a bad experience with the product. All I was doing was explaining the thought process behind why it is how it currently is. But I'm completely open to better solutions.
I think you are right that it will end up YSODing again on the next upgrade as with the v7 version shipping with all the providers, the new package file will copy over the payment providers DLL again containing the same version of Stripe. That is unless I find a means of merging it and internalizing it so that it is only seen by the payment provider.
Using AssemblyLoad event we could either look for an embeded DLL or a DLL in a custom location.
I need to look into it further though as I'm not sure if this would run every time, or just if the assembly can't be found. The later would probably be what we want (although the problem about upgrading to a new version not supported by the provider still stands, but at this point, if you are changing the DLL, I could just assume you know what you are doing and the potential consequences?)
I don't want this to become too complicated and/or advanced. There are so many other tasks with likely higher priority in TeaCommerce, documentation, V8, and Backoffice UI comes to mind :-)
I'd say go with internalizing if you can make that work, or consider go back to loading the Stripe dll outside (instead of ILMerging) of the providers dll. With that, document the fact that the dll might overwrite any existing dll upon TeaCommerce install. It should be easy enough for us (custom provider authors) to reinsert our version of Stripe after TeaCommerce has installed. It would be our responsibility then to ensure the version is compatible with the standard Stripe provider you supply.
Problem with Stripe ILMerged into PaymentProviders
I just went through the process of upgrading from TC 3.2.4 to 3.3.1 and everything seems to work, except for one issue. Our custom Stripe based payment provider uses a newer version of Stripe.net than what's ILMerged into the TeaCommerce.PaymentProviders.dll, and when we add the newer Stripe.net assembly to the site we end up with the same types/classes in the same namespace (Stripe) coming from two different assemblies, and if we use a Stripe identifier anywhere we get compile errors.
"The type 'PaymentIntent' exists in both 'Stripe.net, Version=27.18.0.0, Culture=neutral, PublicKeyToken=null' and 'TeaCommerce.PaymentProviders, Version=3.1.7114.13982, Culture=neutral, PublicKeyToken=null'"
I first thought that we could work around this by using assembly aliasing, but that feature seems to be unavailable with web site projects (and is not ideal anyway). So then maybe the only solution is to recompile the PaymentProviders assembly locally using the correct Stripe assembly? If yes, what is the recommented (easiest) approach to doing this?
ILMerging one third-party assembly into a second third-party assembly might be a less than perfect practice, particulary in Stripe.net's case since it's being updated fairly frequently (far more frequently than TeaCommerce). Thoughts?
-Tor
Hey Tor,
I agree, it's less than ideal. The problem we have with the current packaging system is that there is just no way to manage versions of third party dependencies so if I just include the DLL in my package it'll replace your's downgrading you to an older version. But if we don't include it, then the payment provider won't work without it. We are kind of damned if we do and damned if we don't.
That said, I thought ILMerging was meant to prevent these issue (or maybe I needed to internalize them but I'm sure I hit problems doing that). Best I can suggest for now if you do need to use a newer Stripe.NET DLL is to grab the source from github and compile it manually, not merging the DLLs.
In TC for v8 we'll have nuget packages and so we can have nuget dependencies, but what to do about Umbraco package files is still an issue. I'm open to suggestions though?
PS I have recently switched to ILRepack instead of ILMerge because someone was having an issue with the ILMerged DLL, but ILRepack worked fine. I wonder if internalizing with ILRepack will have the same issues as with ILMerge. I'll have to do some more testing.
Internalizing the Stripe assembly might work, but there's still code smell. I (the user) would have to remember that the standard PaymentProviders assembly is using one version of Stripe, while my code is using another.
I'm not an expert on project configurations, but would it be possible to leave the Stripe dll out of the ILMerged assembly, and instead require me (your user) to supply it (Nuget or otherwise)? If yes, maybe it also would be possible for you to specify (in the project config) minimum/maximum versions of Stripe that are compatible with PaymentProviders?
Maybe something here: https://docs.microsoft.com/en-us/dotnet/framework/deployment/how-the-runtime-locates-assemblies would help?
The problem if we were to not include the Stripe.NET DLL is that now Tea Commerce installs incomplete and as it currently stands, it installs everything and all providers when you install the package. If we don't deploy the Stripe.NET DLL , then it will either YSOD immediately (not a great experience) or just won't show Stripe in the providers list (which as all others with no dependency will be in the list, devs will just assume that Stripe isn't supported, so equally not a great experience).
I appreciate that for people working with Stripe, the ILMerge is a problem, but really, devs including a new Stripe.NET DLL are likely more clued up and able to work around this, than folks who just want to install it and it works.
IIRC it is possible to embed a DLL into another DLL and then you can implement a sort of finder so I think it will look in the AppDomain but if it's not found, ask these finders. So it might be that we have to go down that route and embed it that way. If this is correct, then you could drop a newer DLL in the bin folder and if it sees it, it will use that, but only fallback to the embedded version if it's not found. Of course, this still poses the risk that the payment provider relies on the older code base and if there are breaking changes in the newer DLL, then this will break the provider.
PS In v8, I'm thinking that all providers will be their own package / nuget package and you just install what you need. With that, I think it's more reasonable to exclude a dependency if we must, or better, provider some UI during the install to check / install if not found.
Also, regarding "I (the user) would have to remember that the standard PaymentProviders assembly is using one version of Stripe, while my code is using another." this would potentially be the case regardless as it might be that the API could have changed so really, we don't want you upgrading the DLL the provider uses as it could break the provider.
Really, I think if you want to use a specific version of Stripe.NET, the safest route is to fork the provider and create a custom implementation of it, all compiled against the same version.
In the payment providers repo on github, I have actually broken all the providers up into their own projects so that this is easier to do.
"...we don't want you upgrading the DLL the provider uses as it could break the provider." This is why I thought maybe you could specify in the config which versions the provider code is compatible with.
"...I think if you want to use a specific version of Stripe.NET, the safest route is to fork the provider and create a custom implementation of it, all compiled against the same version."
This is what we have done :-) And inside our provider/assembly everything is fine. But we had a need to reference Stripe in the web site project and that's where the conflict occurred. EDIT: not against the same version, it's our own assembly, side by side with the standard providers assembly.
I'm just giving feedback and trying to be constructive. I can easily solve the issues for us by recompiling the providers assembly, so things are good in that respect. But it will be a thing for us to remember for time there's a Tea Commerce upgrade.
Minor side note, the Tea Commerce upgrade/install did in fact YSOD due to the Stripe conflicts. I was waiting on the installation page forever until I gave up and started debugging. That's when I noticed the compile errors. Hmm, does that mean the YSOD will happen on the next upgrade as well? (until we replace the providers dll with our recompiled version again)?
We are very happy that Tea Commerce now is under active development, and we will be upgrading to Umbraco / TC 8 when the time comes. Keep it up!
-Tor
Hey Tor,
Oh, I'm not dismissing your feedback by any means, it's completely valid and useful to know the issues people are facing. I don't want anyone to have a bad experience with the product. All I was doing was explaining the thought process behind why it is how it currently is. But I'm completely open to better solutions.
I think you are right that it will end up YSODing again on the next upgrade as with the v7 version shipping with all the providers, the new package file will copy over the payment providers DLL again containing the same version of Stripe. That is unless I find a means of merging it and internalizing it so that it is only seen by the payment provider.
This was the potential route I was thinking about https://docs.microsoft.com/en-us/dotnet/framework/app-domains/resolve-assembly-loads
Using AssemblyLoad event we could either look for an embeded DLL or a DLL in a custom location.
I need to look into it further though as I'm not sure if this would run every time, or just if the assembly can't be found. The later would probably be what we want (although the problem about upgrading to a new version not supported by the provider still stands, but at this point, if you are changing the DLL, I could just assume you know what you are doing and the potential consequences?)
I don't want this to become too complicated and/or advanced. There are so many other tasks with likely higher priority in TeaCommerce, documentation, V8, and Backoffice UI comes to mind :-)
I'd say go with internalizing if you can make that work, or consider go back to loading the Stripe dll outside (instead of ILMerging) of the providers dll. With that, document the fact that the dll might overwrite any existing dll upon TeaCommerce install. It should be easy enough for us (custom provider authors) to reinsert our version of Stripe after TeaCommerce has installed. It would be our responsibility then to ensure the version is compatible with the standard Stripe provider you supply.
-Tor
Sounds reasonable. And I agree, the simpler the better :)
I'll have a think of what to do for the next release 👍
Thanks again for your feedback. It really is valued.
is working on a reply...