AVRO-2843: [PHP] Copy composer setup from Apache Thrift

[jjatria]

What is the purpose of the change

This PR copies the composer setup from https://github.com/apache/thrift which Apache successfully uses to publish the Thrift package to packagist.org (https://packagist.org/packages/apache/thrift). This is in an attempt to fix https://issues.apache.org/jira/browse/AVRO-2843

Verifying this change

This change does not add any tests, since it includes no code changes.

Documentation

• Does this pull request introduce a new feature? (yes / no)
• If yes, how is the feature documented? (not applicable / docs / JavaDocs / not documented)

[martin-g]

https://issues.apache.org/jira/browse/AVRO-2752?focusedCommentId=17870165&page=com.atlassian.jira.plugin.system.issuetabpanels%3Acomment-tabpanel#comment-17870165
According to Siad this plugin does something important here! But AFAIU the idea is to use archive > exclude below instead!

[jjatria]

My packagist-fu is almost non-existent, but Thrift also uses a monorepo-based setup, and in their case they don't seem to need that plugin. I'm more than happy to re-instate the plugin if it is actually needed. I'll try to take a look at those failing tests

[adamquaile]

My understanding here is that the monorepo plugin is designed to allow publishing multiple php packages from the same repository. The important part it's currently doing is using the monorepo.json file as a discovery-mechanism of sorts to locate the php source files.

Unless I'm missing something the autoload.psr-4 section of this new version will serve that need instead via PSR-4 autoloading and the archive.exclude with the exclamation mark ensures only the php parts of this repository are included.

I think we could even delete the monorepo.json file too. Edit: I can see it's already deleted

[adamquaile]

I also currently hit install issues with the old version. I'm testing with a brand new empty project, with the following composer.json:

{
    "require": {
        "apache/avro": "dev-main"
    },
    "repositories": [
        {
            "type": "vcs",
            "url": "https://github.com/apache/avro"
        }
    ],
    "config": {
        "allow-plugins": {
            "beberlei/composer-monorepo-plugin": true
        }
    }
}

composer install
No composer.lock file present. Updating dependencies to latest instead of installing from lock file. See https://getcomposer.org/install for more information.
Loading composer repositories with package information
Updating dependencies
Lock file operations: 2 installs, 0 updates, 0 removals
  - Locking apache/avro (dev-main 35b01df)
  - Locking beberlei/composer-monorepo-plugin (v0.16.5)
Writing lock file
Installing dependencies from lock file (including require-dev)
Package operations: 1 install, 1 update, 0 removals
  - Installing beberlei/composer-monorepo-plugin (v0.16.5): Extracting archive
  - Upgrading apache/avro (dev-AVRO-2843-packagist 4c1ec59 => dev-main 35b01df): Extracting archive
Generating autoload files
Generating autoload files for monorepo sub-packages with dev-dependencies.
PHP Fatal error:  Declaration of Monorepo\Composer\EventDispatcher::dispatch($eventName, ?Composer\EventDispatcher\Event $event = null) must be compatible with Composer\EventDispatcher\EventDispatcher::dispatch(?string $eventName, ?Composer\EventDispatcher\Event $event = null): int in /Users/aquaile/Development/avro-tests/vendor/beberlei/composer-monorepo-plugin/src/main/Monorepo/Composer/EventDispatcher.php on line 9

Fatal error: Declaration of Monorepo\Composer\EventDispatcher::dispatch($eventName, ?Composer\EventDispatcher\Event $event = null) must be compatible with Composer\EventDispatcher\EventDispatcher::dispatch(?string $eventName, ?Composer\EventDispatcher\Event $event = null): int in /Users/aquaile/Development/avro-tests/vendor/beberlei/composer-monorepo-plugin/src/main/Monorepo/Composer/EventDispatcher.php on line 9

Pointing to the new branch with this works:

{
    "require": {
        "apache/avro": "dev-AVRO-2843-packagist"
    },
    "repositories": [
        {
            "type": "vcs",
            "url": "https://github.com/cv-library/avro"
        }
    ]
}

[jjatria]

The linting action was failing because without the monorepo plugin composer install installs the vendor directory in the root of the repo rather than inside lang/php. Updating the path in PHP's build.sh seems to fix it locally, but let's see if they also helps in the actions.

[jjatria]

I made one additional change to lang/php/test/test_helper.php so it (like build.sh) points to the vendor directory at the repo root level rather than one inside lang/php. Locally, that allows the tests to pass, although I get this as a summary:

OK, but incomplete, skipped, or risky tests!
Tests: 415, Assertions: 969, Risky: 415.

Most of the "risky" tests complain like this:

This test does not have a @covers annotation but is expected to have one

but that sees to me like it should be addressed on a separate PR to keep the scope limited.

[jjatria]

The risky warnings come from https://github.com/apache/avro/blob/main/lang/php/phpunit.xml#L22, which we could either set to false for now, or we could add the annotations it expects. Either way, this seems to be ready :)

[jjatria]

Although this PR does what it says on the tin (it copies the working setup from the Thrift repository) one thing @adamquaile spotted while working on this is that the exclude directive does not do what we expected it to do. That is, if you install the package with composer on a target machine, a full copy of the repository will end up in that target machine's file system. One would expect this issue to also affect Thrift.

The monorepo plugin would, one assumes, fix this problem, but if we wanted to re-instate that we would have to figure out what about the original setup was not working (and preventing eg. @adamquaile from installing from main).

[thiagorb]

@jjatria It looks like the monorepo setup doesn't work as assumed.

Just tested this:

composer init
...
composer config repositories.repo-name vcs https://github.com/apache/avro
composer require apache/avro:dev-main

After that, I can still see all the files in this repo by running:

find vendor/apache/avro/

So, I don't see a reason to keep the monorepo usage.

[skystebnicki]

Thanks for doing this @jjatria, been struggling with this issue for a while now. Any idea when it will be merged?

[jjatria]

@skystebnicki Not really. Other than my comment about the keywords to use for Avro in the composer.json file (currently set to "RPC"), I think this is ready to be merged, but the merging decision is up to the maintainers. Perhaps @martin-g has more insights?

[martin-g]

I have no experience with these matters and I totally depend on you here.
Recently @jjatria also said that he does not have much experience with PHP, so I expected someone else from the community to review and approve the changes.