Bug 1982616 - Review Request: php-beberlei-assert - Thin assertion library for input validation in business models
Summary: Review Request: php-beberlei-assert - Thin assertion library for input valida...
Keywords:
Status: CLOSED NOTABUG
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Nobody's working on this, feel free to take it
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On:
Blocks: FE-DEADREVIEW 1982648 1982651 1982652
TreeView+ depends on / blocked
 
Reported: 2021-07-15 10:01 UTC by Christopher Engelhard
Modified: 2023-09-06 00:45 UTC (History)
3 users (show)

Fixed In Version:
Doc Type: If docs needed, set a value
Doc Text:
Clone Of:
Environment:
Last Closed: 2023-09-06 00:45:27 UTC
Type: ---
Embargoed:


Attachments (Terms of Use)
makesrc.sh (782 bytes, text/plain)
2021-07-20 07:18 UTC, Remi Collet
no flags Details

Description Christopher Engelhard 2021-07-15 10:01:00 UTC
Spec URL: https://download.copr.fedorainfracloud.org/results/lcts/nextcloud/fedora-rawhide-x86_64/02329601-php-beberlei-assert/php-beberlei-assert.spec
SRPM URL: https://download.copr.fedorainfracloud.org/results/lcts/nextcloud/fedora-rawhide-x86_64/02329601-php-beberlei-assert/php-beberlei-assert-3.3.1-1.fc35.src.rpm

Description: A simple php library which contains assertions and guard methods for input validation (not filtering!) in business-model, libraries and application low-level code. The library can be used to implement pre-/post-conditions on input data.

Since this package is set up to use rpmautospec, rpmlint/fedora-review will complain about missing dist tags & macros in changelog. These are spurious errors/warnings.

This package is available from the lcts/nextcloud Copr, so you can also test it using 'fedora-review --copr-build 2329601'


Fedora Account System Username: lcts

Comment 1 Otto Liljalaakso 2021-07-19 11:37:42 UTC
I have reviewed this package.Two findings:

1.
> License:        BSD
That is what LICENSE and composer.json files say.
Source files claim to be under MIT
and then refer to nonexistent file LICENSE.txt.
Even though any possible interpretation is allowed in Fedora,
the license is unclear.
Upstream must be contacted about this unclarity
so that LICENSE file and source file headers can be made to agree.
However, since it is quite clear that BSD 2-Clause is the intended license,
I do consider it necessary to wait for a fix
before this package can be added to fedora. 

2.
> : No tests implemented
Same comment as for php-leagure-uri-interfaces [1]:
There are tests upstream, but they remove them from Git archives.
I would like to seem them ran, but not sure how to make that happen.
[1]: https://bugzilla.redhat.com/show_bug.cgi?id=1982635

Comment 2 Remi Collet 2021-07-20 05:06:01 UTC
Small notices

- yes, running upstream test suite make sense

- PSR-0 tree doesn't make sense anymore, nobody will rely on a single PSR-0 tree

It is fine to use /usr/share/php/Assert, but this can raise conflicts, so
good be a good idea to introduce a fake "vendor" directory
ex: /usr/share/php/beberlei/Assert

- PSR-4 autoload is fine, but a classmap one can be faster

(composer itself uses classmap for production optimized autoloader)

- Documentation usually includes *.md files

Comment 3 Remi Collet 2021-07-20 07:09:23 UTC
Here is some possible changes

+++ php-beberlei-assert.spec	2021-07-20 09:07:23.444053501 +0200
@@ -11,7 +11,7 @@
 
 # PHP namespace and directory
 %global ns_project  Assert
-%global ns_dir      %( echo "%{ns_project}" | sed  's|\\\\|\/|g' )
+%global ns_dir      %{vendor}/%( echo "%{ns_project}" | sed  's|\\\\|\/|g' )
 
 # Github
 %global gh_vendor   beberlei
@@ -20,7 +20,7 @@
 %global scommit     %(c=%{commit}; echo ${c:0:7})
 
 # tests
-%global with_tests  1
+%bcond_without tests
 
 #-- PREAMBLE ------------------------------------------------------------------#
 Name:           php-%{vendor}-%{project}
@@ -30,7 +30,8 @@
 
 License:        BSD
 URL:            https://github.com/%{gh_vendor}/%{gh_project}
-Source0:        https://github.com/%{gh_vendor}/%{gh_project}/archive/%{commit}/%{gh_project}-%{version}-%{scommit}.tar.gz
+Source0:        %{name}-%{version}-%{scommit}.tgz
+Source1:        makesrc.sh
 
 BuildArch:      noarch
 
@@ -57,8 +58,18 @@
 BuildRequires:  %{_bindir}/php
 BuildRequires:  php(language) >= 7.0
 
-%if 0%{?with_tests}
-# for tests
+%if %{with tests}
+BuildRequires:  phpunit9
+BuildRequires:  php-simplexml
+BuildRequires:  php-mbstring
+BuildRequires:  php-ctype
+BuildRequires:  php-json
+BuildRequires:  php-intl
+BuildRequires:  php-date
+BuildRequires:  php-filter
+BuildRequires:  php-pcre
+BuildRequires:  php-reflection
+BuildRequires:  php-spl
 %endif
 
 # composer provides
@@ -94,13 +105,9 @@
 \Fedora\Autoloader\Autoload::addPsr4('%{ns_project}', __DIR__);
 
 // files & dependencies
-\Fedora\Autoloader\Dependencies::required(array(
+\Fedora\Autoloader\Dependencies::required([
   __DIR__.'/functions.php',
-));
-
-\Fedora\Autoloader\Dependencies::optional(array(
-  // no optional dependencies
-));
+]);
 EOF
 
 %check
@@ -113,16 +120,29 @@
         ? 0 : 1
     );
 "
-%if 0%{?with_tests}
+%if %{with tests}
 : Run tests
-: No tests implemented
+mkdir vendor
+cat << 'EOF' | tee vendor/autoload.php
+<?php
+require_once '%{buildroot}%{_datadir}/php/%{ns_dir}/autoload.php';
+\Fedora\Autoloader\Autoload::addPsr4('%{ns_project}\\Tests', dirname(__DIR__).'/tests/Assert/Tests');
+\Fedora\Autoloader\Dependencies::required([
+	'%{_datadir}/php/Yoast/PHPUnitPolyfills/autoload.php',
+    dirname(__DIR__).'/tests/Assert/Tests/Fixtures/functions.php',
+]);
+EOF
+
+phpunit9 --migrate-configuration
+phpunit9 --verbose
 %endif
 
 #-- FILES ---------------------------------------------------------------------#
 %files
 %license LICENSE
 %doc composer.json
-%{_datadir}/php/%{ns_dir}
+%doc *md
+%{_datadir}/php/%{vendor}
 
 
 #-- CHANGELOG -----------------------------------------------------------------#



Using this, during the build:

+ phpunit9 --verbose
PHPUnit 9.5.7 by Sebastian Bergmann and contributors.

Runtime:       PHP 7.4.21
Configuration: /dev/shm/extras/BUILD/assert-5e721d7e937ca3ba2cdec1e1adf195f9e5188372/phpunit.xml.dist

................................................................................................................................................................... 163 / 513 ( 31%)
................................................................................................................................................................... 326 / 513 ( 63%)
................................................................................................................................................................... 489 / 513 ( 95%)
........................                                                                                                                                            513 / 513 (100%)

Time: 00:00.026, Memory: 10.00 MB

OK (513 tests, 904 assertions)

Comment 4 Remi Collet 2021-07-20 07:13:09 UTC
In above, missing the BuildRequires for yoast/phpunit-polyfills

Comment 5 Remi Collet 2021-07-20 07:18:15 UTC
Created attachment 1803525 [details]
makesrc.sh

Comment 6 Christopher Engelhard 2021-07-21 11:34:20 UTC
Thanks, Otto, Remi for the comments.

> 1.
> > License:        BSD
> That is what LICENSE and composer.json files say.
> Source files claim to be under MIT
> and then refer to nonexistent file LICENSE.txt.
> Even though any possible interpretation is allowed in Fedora,
> the license is unclear.
> Upstream must be contacted about this unclarity
> so that LICENSE file and source file headers can be made to agree.
> However, since it is quite clear that BSD 2-Clause is the intended license,
> I do consider it necessary to wait for a fix
> before this package can be added to fedora. 

That makes sense. There is already an old issue regarding this upstream [1], which unfortunately hasn't
been responded to. I'll see if I can get hold of the owner, they're still quite active
on Github.

> 2.
> > : No tests implemented
> Same comment as for php-leagure-uri-interfaces [1]:
> There are tests upstream, but they remove them from Git archives.
> I would like to seem them ran, but not sure how to make that happen.
> [1]: https://bugzilla.redhat.com/show_bug.cgi?id=1982635

Here I can use a local git checkout to create a source tarball that includes the tests (See for example here: [2]), as suggested by Remi.

[1] https://github.com/beberlei/assert/issues/221
[2] https://src.fedoraproject.org/rpms/php-guzzlehttp-guzzle6/blob/rawhide/f/php-guzzlehttp-guzzle6.spec

Comment 7 Christopher Engelhard 2021-08-05 16:05:15 UTC
Spec URL: https://download.copr.fedorainfracloud.org/results/lcts/nextcloud/fedora-rawhide-x86_64/02364182-php-beberlei-assert/php-beberlei-assert.spec
SRPM URL: https://download.copr.fedorainfracloud.org/results/lcts/nextcloud/fedora-rawhide-x86_64/02364182-php-beberlei-assert/php-beberlei-assert-3.3.1-1.fc35.src.rpm


Quick update:
I've implemented the tests and moved the install directory to beberlei/Assert using Remi's patch (Thanks for that).

Interestingly, some tests fail only on s390x! with 'IntlException: Constructor failed'. Must be somehow related to php-intl, but I haven't figured out why yet.

I haven't gotten hold of the developer yet re: the license issue, I'll set NEEDINFO as soon as that is cleared up.

Comment 8 Package Review 2022-08-06 00:45:19 UTC
This is an automatic check from review-stats script.

This review request ticket hasn't been updated for some time, but it seems
that the review is still being working out by you. If this is right, please
respond to this comment clearing the NEEDINFO flag and try to reach out the
submitter to proceed with the review.

If you're not interested in reviewing this ticket anymore, please clear the
fedora-review flag and reset the assignee, so that a new reviewer can take
this ticket.

Without any reply, this request will shortly be resetted.

Comment 9 Otto Liljalaakso 2022-08-06 17:42:20 UTC
Well, it has been a year, and it is the submitter that should perform the missing work.
I'll clear fedora-review and unassign this review.
In case there is progress, you can contact me again and I can finish the review.

Comment 10 Package Review 2023-08-07 00:45:25 UTC
This is an automatic check from review-stats script.

This review request ticket hasn't been updated for some time. We're sorry
it is taking so long. If you're still interested in packaging this software
into Fedora repositories, please respond to this comment clearing the
NEEDINFO flag.

You may want to update the specfile and the src.rpm to the latest version
available and to propose a review swap on Fedora devel mailing list to increase
chances to have your package reviewed. If this is your first package and you
need a sponsor, you may want to post some informal reviews. Read more at
https://fedoraproject.org/wiki/How_to_get_sponsored_into_the_packager_group.

Without any reply, this request will shortly be considered abandoned
and will be closed.
Thank you for your patience.

Comment 11 Package Review 2023-09-06 00:45:27 UTC
This is an automatic action taken by review-stats script.

The ticket submitter failed to clear the NEEDINFO flag in a month.
As per https://fedoraproject.org/wiki/Policy_for_stalled_package_reviews
we consider this ticket as DEADREVIEW and proceed to close it.


Note You need to log in before you can comment on or make changes to this bug.