Reviewed. This is my first php review, so please giv guidance if some of my comments are misguided.

Two blocking issues and some non-blocking found:

1.
> License:        GPLv3+
Both the LICENSE file and composer.json say it is actually MIT.
Please fix.

2.
From fedora-review:
Note: Directories without known owners: /usr/share/php/League
This is a MUST item in the guidelines,
so it must be fixed.
I am not sure how vendor directory ownership is usually handed in php packages.
I suppose all packages from League could share the ownership?
Then this would do it: %dir %{_datadir}/php/League
Reference: https://docs.fedoraproject.org/en-US/packaging-guidelines/#_file_and_directory_ownership

> # Please preserve the changelog entries
I do not understand this request in combination with %autochangelog

3.
Guidelines say
"The composer.json file is not used,
and should be installed as %doc
as it provides useful information about the package and its dependencies.".
I am not certain how to interpret this statement.
Since it says should, I do not think it is a blocking issue.
However, it clearly recommends installing the file as %doc
instead of dropping it completely
like is done here.
Perhaps it could be added as %doc as suggested?
Reference: https://docs.fedoraproject.org/en-US/packaging-guidelines/PHP/#_file_placement

4.
> : No tests implemented
This one is strange.
There is a dev dependency to phpunit in composer.json,
there is phpunit.xml
whose syntax is do not completely understand
but I think it is trying to say "execute everything from src/ that ends with Test.php"
and there are two such files under src/Idna.
But upstream removes them from the Git archives via .gitattributes statements.
Since the guidelines say test suites should be run,
it would be good to contact upstream
about how to make running the tests possible.
This is not a blocking review issue,
because running tests is just a should
and upstream has intentionally stripped them.
In any case, the comment should describe this situation.

And two that are more questions than review findings:

1.
A release is packaged, but it downloaded by commit id rather than tag or GitHub release. Nothing wrong with that, I am just curious why it is done like that here?

2.
> Autoloader: /usr/share/php/League/UriInterfaces/autoload.php
I am surprised that this information is important enough
to be added to description.
Why?
Also, there would be a slight readability gain
from formatting this differently,
so it would not look so much like a specfile tag.

Thanks for the review. I'm working on the test issue and will set NEEDINFO once I have a new build including all fixes.

(In reply to Otto Urpelainen from comment #1)
> 2.
> From fedora-review:
> Note: Directories without known owners: /usr/share/php/League
> This is a MUST item in the guidelines,
> so it must be fixed.
> I am not sure how vendor directory ownership is usually handed in php
> packages.
> I suppose all packages from League could share the ownership?
> Then this would do it: %dir %{_datadir}/php/League
> Reference:
> https://docs.fedoraproject.org/en-US/packaging-guidelines/
> #_file_and_directory_ownership

Yes. Other packages in the \League namespace already own /usr/share/php/League, but since this one doesn't depend on any of them (unlike e.g. php-league-uri) it should own it as well. Will fix.

> 
> > # Please preserve the changelog entries
> I do not understand this request in combination with %autochangelog

Once the package is actually build, rpmautospec will replace the macro with the actually (build from git) changelog. Hence, anybody messing around with a specfile they got from a source package should still preserve that changelog if possible.
 
> 3.
> Guidelines say
> "The composer.json file is not used,
> and should be installed as %doc
> as it provides useful information about the package and its dependencies.".
> I am not certain how to interpret this statement.
> Since it says should, I do not think it is a blocking issue.
> However, it clearly recommends installing the file as %doc
> instead of dropping it completely
> like is done here.
> Perhaps it could be added as %doc as suggested?

This is an oversight on my part. The composer file is useful (even if you don't use composer) because it e.g. tells the user which namespaces the PHP classes are in, where to find further documentation etc. I'll include it.

> 4.
> > : No tests implemented
I'll include the tests by using a git checkout instead of the prepare tarball, as discussed here https://bugzilla.redhat.com/show_bug.cgi?id=1982616

(In reply to Otto Urpelainen from comment #2)
> 1.
> A release is packaged, but it downloaded by commit id rather than tag or
> GitHub release. Nothing wrong with that, I am just curious why it is done
> like that here?

Because people most often look for releases etc. via composer/packagist.org, and that references packages via commit id: https://packagist.org/packages/beberlei/assert
Packaging it this way a) makes life easier because all composer packages look very similar and b) ensures that no matter what upstream does in their git repo, the package people get via composer is the same they get via the repos.
 
> 2.
> > Autoloader: /usr/share/php/League/UriInterfaces/autoload.php
> I am surprised that this information is important enough
> to be added to description.
> Why?

Because you need to know where the autoloader is in order to actually use this package in a project, and its location standardized. No searchpath, unfortunately.

> Also, there would be a slight readability gain
> from formatting this differently,
> so it would not look so much like a specfile tag.

Good point, I hadn't noticed that.

Thank you for explaining all this. I'll take another look when you have the next iteration ready.

Spec URL: https://download.copr.fedorainfracloud.org/results/lcts/nextcloud/fedora-rawhide-x86_64/02339704-php-league-uri-interfaces/php-league-uri-interfaces.spec
SRPM URL: https://download.copr.fedorainfracloud.org/results/lcts/nextcloud/fedora-rawhide-x86_64/02339704-php-league-uri-interfaces/php-league-uri-interfaces-2.3.0-1.fc35.src.rpm

'fedora-review --copr-build 2339704'

New build. fedora-review will now probably complain about /usr/share/php/League being also owned by other php-league-* packages. This is expected, as discussed above.
The build only runs the phpunit tests, not the phpstan static analysis as Fedora doesn't have phpstan in the repos. That is why the specfile BuildRequires: are different from those in composer.json.

Since I'm now building the source from a full git checkout I've also added the .md files as documentation that were missing from the previous tarball.

Thank you, all items from the previous round are now fixed.

The only quibble I have is that the new way of creating Source0
had led to the test files being installed together with the library.
You may want to remove them during install or %exclude them.
This is so minor that I do not see any point
in requiring more review rounds.

Review passed, thank you for packaging this
and for splitting bundled dependencies.

I will take another package review from the owncloud set
as soon as I have time.


(In reply to Christopher Engelhard from comment #5)
> 
> New build. fedora-review will now probably complain about
> /usr/share/php/League being also owned by other php-league-* packages. This
> is expected, as discussed above.

(In reply to Christopher Engelhard from comment #5)
> New build. fedora-review will now probably complain about
> /usr/share/php/League being also owned by other php-league-* packages. This
> is expected, as discussed above.

What I meant to say about this was
that here's another place where fedora-review needs fixing.
There is no MUST item for
"Package does not own files or directories owned by other packages,"
it is the opposite, the guidelines explicitly allow shared ownership.
Listing the shared cases is good service,
so that weird situations can be spotted,
but the wording is badly off.

(fedscm-admin):  The Pagure repository was created at https://src.fedoraproject.org/rpms/php-league-uri-interfaces