eileenmcnaughton / nz.co.fuzion.omnipaymultiprocessor

Omnipay Multi Processor Payment Processor For CiviCRM
Other
14 stars 44 forks source link

Fix a problem with php 8 and civicrm 5.67.1 #261

Open jaapjansma opened 11 months ago

jaapjansma commented 11 months ago

When I installed this in our demo environment I discovered a compatibility issue with php 8 and civicrm 5.67.1 and Drupal 10.

Not sure whether this breaks other compatibility, such as Drupal 7, or wordpress or older versions of CiviCRM.

eileenmcnaughton commented 11 months ago

Thanks @jaapjansma - these files are pulled in by composer so I'm guessing we need to change the version of symfony? There is a big-picture issue around how extensions with composer packages install - here I have committed them which is easy for some frameworks but tricky for others. So consistent with that we would update any package within the extension by doing a composer update & committing the results. I'm just wonder what the various versions of symfony that are involved are.

I might raise over on dev that WMF uses the composer merge plugin package (& then does not commit any vendor directories to our git) - I don't know if that package could be used more broadly

jaapjansma commented 11 months ago

@eileenmcnaughton I am not very familiar with composer and extension within a framework. But the composer merge plugin sounds reasonable. Maybe @totten or @mattwire have any thoughts that.

stesi561 commented 8 months ago

This PR is now out of date but this issue is related: https://github.com/eileenmcnaughton/nz.co.fuzion.omnipaymultiprocessor/issues/259

stesi561 commented 8 months ago

@eileenmcnaughton is it worth updating this PR to update symfony/http-foundation or is there ongoing work here to address the underlying issue? I think currently for D10 we'd need symfony/http-foundation 6.4.x but I'm keen to explore options for composer merge plugin package.

jaapjansma commented 2 months ago

@eileenmcnaughton I have updated this PR so it can be merged.

nganivet commented 1 month ago

@eileenmcnaughton Patch confirmed working on D10 / 5.75 / latest Omnipay release

eileenmcnaughton commented 1 month ago

thanks @nganivet - but we do need to figure out how to do this via composer rather than hack the package (we just do the composer update & commit the updated files currently)

nganivet commented 1 month ago

Thanks for the quick response. Would it be possible to do this with composer patches? I recently encountered https://civicrm.stackexchange.com/questions/47989/db-error-unknown-database-when-creating-a-civimail-mailing and this seems to imply we can apply patches to dependencies. Could we roll this patch into CiviCRM core?

jaapjansma commented 2 weeks ago

I have created a new extension which contains the Omnipay Mollie and which works with the current versions of civicrm. See here: https://civicrm.org/extensions/omnipay-payment-processor

eileenmcnaughton commented 2 weeks ago

So the underlying problems are

1) it is hard to support composer in extensions 2) we don't have a way to denote the supported php versions for an extension.

I had a conversation about 2 with @totten last week & based on that I have added tags to the info.xml to show supported php versions (specifically excluding 7.4) and updated this to support php 8.0x only - which will address this issue (at the cost of php 7.4) - but I think that is less bad that having a cut down fork of this extension with the same name

Obviously maintaining extensions is a lot of work

eileenmcnaughton commented 2 weeks ago

Ok - I created https://lab.civicrm.org/dev/core/-/issues/5581 to deal with the extension ability to flag supported php versions - it's not that hard to support different php versions (or different civi versions) - but it is often hard to support them in the SAME release of an extension & the trade-offs to do that often lead to problems in extension maintenance. We can deal with that for CiviCRM versions - ie I simply say 'if you want to run CiviCRM 5.60 with my extension use an older release of my extension' - but we want to get there for php versions too

jaapjansma commented 2 weeks ago

Yes you are right. It is hard to deal with composer stuff in an extension. The PDF Api also has composer.json in the extension folder. Ideally CiviCRM runs composer install when installing an extension and checks it with the dependencies in the composer.json from civicrm it self.

nganivet commented 2 weeks ago

Cross-ref: https://github.com/totten/civix/issues/234