Project60 / org.project60.banking

CiviCRM banking extension
18 stars 38 forks source link

Status of the extension with composer/Drupal 9 #361

Closed VangelisP closed 1 year ago

VangelisP commented 2 years ago

Hi there! What is the status of the extension in regards to Drupal 9 & Composer?

Right now, as the extension is missing a composer.json file, we're downloading the package like that:

"require": {
    "civicrm/civibanking": "0.8-alpha9",
    ...
},
"repositories": [
    {
        "type": "package",
        "package" : {
            "name": "civicrm/civibanking",
            "version" : "0.8-alpha9",
            "type":"civicrm-extension",
            "dist": {
                "type": "zip",
                "url": "https://github.com/Project60/org.project60.banking/archive/refs/tags/0.8-alpha9.zip"
            }
        }
    },
    ...
]

The extension downloads fine on this step, however, the problem with that is that because it's being included in a subfolder called extension, the composer plugin civicrm-asset-plugin is not picking it up thus not extracting css/js files to the web/libraries/civicrm folder, therefore, the UI seems broken.

One first approach would be to have a release with the contents of the extension folder directly on the root and also add composer.json to that root folder.

Thoughts?

sluc23 commented 2 years ago

@bjendres any plans on adapting this extension to be more composer-friendly ? I think the folder structure of this repo has been a legacy issue for a long time, we could bypassed with some tricks for D7/WP installations.. but for D9, based on composer, this is a blocking issue right now.

tx!

bjendres commented 2 years ago

Sounds good to me, could you create a PR?

But... don't we have to include the two bundled libraries (eval-math and jsoneditor) as a composer dependency as well?

VangelisP commented 2 years ago

@bjendres in regards to your question about the 2 libraries, you have 2 options:

1) Keep it as-is, CiviCRM works fine by publishing the CSS/JS assets that already exist on the repository via the civicrm-asset-plugin, so you don't really need to do anything at the moment. 2) Try to add the as external library requirements via composer.json, but that would mean that it needs more work.

bjendres commented 2 years ago

Thanks @VangelisP. Option 2 sounds good to me, but I don't have the time or means to do this.

If somebody wants to create a PR, you'd be very welcome.

bjendres commented 2 years ago

One first approach would be to have a release with the contents of the extension folder directly on the root and also add composer.json to that root folder. Thoughts?

I agree, that's something we should do, see also #197, but it would break all open branches and PRs.

How about we schedule this for after the 0.8 release?

VangelisP commented 2 years ago

I'll do some tests adding the 2 libraries via composer.json

bjendres commented 2 years ago

We (@jensschuppe, @VangelisP and @bjendres) decided to normalise the repository structure with version 1.0, but we'll have to close all outstanding pull requests.

bjendres commented 1 year ago

So, now the master of CiviBanking has the "normal" (composer-friendly) directory structure.

It would be great if you could create a new PR adding the composer files.

VangelisP commented 1 year ago

Fantastic work Bjorn ! I will add the PR as soon as possible!

VangelisP commented 1 year ago

PR is ready: https://github.com/Project60/org.project60.banking/pull/377

bjendres commented 1 year ago

PR is ready: #377

Merged it, thanks. Can we close here, too, then?

VangelisP commented 1 year ago

Yes! All good !