Project60 / org.project60.sepa

SEPA direct debit integration with civicrm
19 stars 46 forks source link

Define permissions in the correct format (with `label` and `description`) #702

Closed jensschuppe closed 3 months ago

jensschuppe commented 3 months ago

The current method of defining permissions is deprecated in CiviCRM 5.71.

bjendres commented 3 months ago

@jensschuppe thanks. Is this compatible with CiviCRM 5.43 or does the minimum version have to be adjusted?

jensschuppe commented 3 months ago

This should be compatible with 5.43 (and older, even with 4.7), see https://github.com/civicrm/civicrm-core/blob/5.43/CRM/Core/Permission/Base.php#L391-L416 for the evaluating code in this version.

The original code allowed both, a string label, or an array with label as first and description as second element. The array format is now required, with associative keys (label and description). If defined in that order, the code is backwards-compatible with old versions.

Support for 5.43 ended in November 2021, though, so I don't think any new versions of CiviSEPA have to be compatible. I see, you just asked because of the minimum version in the info.xml.

bjendres commented 3 months ago

This should be compatible with 5.43 (and older, even with 4.7

Great, thanks.

~Support for 5.43 ended in November 2021, though, so I don't think any new versions of CiviSEPA have to be compatible.~ I see, you just asked because of the minimum version in the info.xml.

For some context: There are still organisations using even older CiviCRM versions, mostly the really large organisations. For those, upgrading CiviCRM is >3 months of work, and we should really try to maintain support for older versions until there's a really good reason not to.

bjendres commented 3 months ago

@jensschuppe would you backport this to 1.9.x?

jensschuppe commented 3 months ago

@jensschuppe would you backport this to 1.9.x?

Yes, that would be useful, as 1.10.x is not even in beta yet. So this will have to be merged into master for 1.10.x and cherry-picked to 1.9.x.

bjendres commented 3 months ago

Merged to master, thanks.

@jensschuppe Please let me know once it's been tested in a real-life scenario, and I'll create a 1.10-alpha5 and cherry-pick it for an 1.9.2.

jensschuppe commented 3 months ago

"Testing" would only involve checking that the permissions are still registered, which I can confirm for a local Drupal environment.

bjendres commented 3 months ago

"Testing" would only involve checking that the permissions are still registered, which I can confirm for a local Drupal environment.

Ok, thanks. So should we wait for ~1 week to see if there's issues, or release in 1.9.2 asap?

jensschuppe commented 3 months ago

I don't think anyone will actually use the master branch, so this won't get any more review. Let's just release a new version with it.

pminf commented 1 month ago

Hey, we don't use master and would be happy if you release a patch version for 1.9.x. It seems that 1.10.x is still some time away.

bjendres commented 1 month ago

It seems that 1.10.x is still some time away.

I don't think so, it's at 1.10-beta5

Hey, we ... would be happy if you release a patch version for 1.9.x.

I'm releasing a 1.9.2 for you :)

francescbassas commented 1 month ago

I can see 1.9.2 here https://github.com/Project60/org.project60.sepa/releases/tag/1.9.2 but not in https://civicrm.org/extensions/civisepa-sepa-direct-debit-extension. Is there anything left to do?

bjendres commented 1 month ago

Is there anything left to do?

Yes, it generates release drafts in the extension directory, that I have to manually publish. Unfortunately that takes some hours, so I don't want to wait - and then I sometimes forget - sorry.

Anyway, they should be up now.

francescbassas commented 1 month ago

Thanks!!

imatge