Bug 1131731 - Review Request: php-pimple - A simple dependency injection container for PHP
Summary: Review Request: php-pimple - A simple dependency injection container for PHP
Status: CLOSED NEXTRELEASE
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Remi Collet
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Keywords:
Depends On:
Blocks:
TreeView+ depends on / blocked
 
Reported: 2014-08-19 23:43 UTC by Shawn Iwinski
Modified: 2015-08-22 19:25 UTC (History)
2 users (show)

(edit)
Clone Of:
(edit)
Last Closed: 2015-08-05 01:29:05 UTC
fedora: fedora-review+
gwync: fedora-cvs+


Attachments (Terms of Use)
phpci.log (5.06 KB, text/plain)
2014-08-26 05:58 UTC, Remi Collet
no flags Details
review.txt (7.39 KB, text/plain)
2014-08-26 05:58 UTC, Remi Collet
no flags Details
phpci.log (6.23 KB, text/plain)
2015-08-02 18:04 UTC, Remi Collet
no flags Details
review.txt (7.83 KB, text/plain)
2015-08-02 18:04 UTC, Remi Collet
no flags Details

Description Shawn Iwinski 2014-08-19 23:43:03 UTC
Spec URL: https://raw.githubusercontent.com/siwinski/rpms/7adf01f466e155430c17ef8e07cefcaadeb3e3ef/php-pimple.spec

SRPM URL: https://siwinski.fedorapeople.org/SRPMS/php-pimple-3.0.0-1.fc20.src.rpm

Description: A simple dependency injection container for PHP.


Fedora Account System Username: siwinski



Note: This package obsoletes php-Pimple and provide both the lib and the extension.

COPR build: http://copr.fedoraproject.org/coprs/siwinski/php-experimental/build/24278/

Comment 1 Remi Collet 2014-08-23 16:22:04 UTC
quick notes (before review)

In description:
WARNING: %{_datadir}/php/Pimple.php is only provided ...
Should be %{_datadir}/php/Pimple/Pimple.php (which is really provided)

This compat file should probably have: include __DIR__.'/Container.php';
for project which use old version without autoloader.

As discussed earlier (for guzzle), see my comment about the autoloader and PSR-0/PSR-4 order issue for local build.

   echo "n" | make test

Cleaner to use:

   export NO_INTERACTION=1
   make test

Perhaps (need to be checked), also need 

   export REPORT_EXIT_STATUS=1

As for twig, I think having the minimal load test "outside" the %if %{with_tests} seems better.

Using "make test" (for ZTS) will imply to wait for PHP 5.5.16 push in updates.

Comment 2 Remi Collet 2014-08-23 16:24:11 UTC
I confirm, you must use

   make test NO_INTERACTION=1 REPORT_EXIT_STATUS=1

Else failure will not break the build.

Comment 4 Remi Collet 2014-08-26 05:58:23 UTC
Created attachment 930739 [details]
phpci.log

phpCompatInfo version 3.3.0

Comment 5 Remi Collet 2014-08-26 05:58:52 UTC
Created attachment 930740 [details]
review.txt

Generated by fedora-review 0.5.1 (bb9bf27) last change: 2013-12-13

Comment 6 Remi Collet 2014-08-26 06:06:08 UTC
[!]: Requires correct, justified where necessary.

php-reflection detected by phpcompatinfo
Ok, this one is always present, but, to be consistent, 
as you BR it, you should also require it. (minor)

[!]: Package functions as described.

Pimple/Pimple.php compatibility class is unusable

Without the C extension, the include must come before the class_alias, else:

    Warning: Class 'Pimple\Container' not found in /usr/share/php/Pimple/Pimple.php on line 10


"BUT", with the C extension

    Fatal error: Cannot redeclare class Pimple\Container in /usr/share/php/Pimple/Container.php on line 34

Indeed, Pimple\Container is provided by the C extension, the .php file is only for compatibility when the C extension not available.

And making the include conditional, like:

    if (!extension_loaded("pimple")) {
        include __DIR__ . '/Container.php';
    }
    class_alias('Pimple\Container', 'Pimple');

Doesn't work...

    Warning: First argument of class_alias() must be a name of user defined class in /usr/share/php/Pimple/Pimple.php on line 13


This means:
- the PHP library is not needed with the C extension
- the compatibility cannot be provided (so probably php-pimple should only obsolete php-Pimple in rawhide)


Any other solution welcome...
For now, I don't really have the "right" answer to this issue.

Comment 9 Remi Collet 2015-08-01 08:29:31 UTC
We are definitively late for this one... sorry :/

Can you please update to 3.0.1 ?

The issue (comment #6) was only related to the compat stuff.

As this is no more provided (and we have php-pimple1 in the repo) it seems much better.

I think you can drop the conflicts.
Both can be installed.

If consumer use some autoloader (the one provided, of any other one), it will never be used, as all the classes and interfaces are provided by the extension.

Of course a direct include of the class won't work, but this is expected.

And so, you can suggest to use the extension (in the library package) to improve speed.

Comment 11 Remi Collet 2015-08-02 18:04:34 UTC
Created attachment 1058584 [details]
phpci.log

Comment 12 Remi Collet 2015-08-02 18:04:58 UTC
Created attachment 1058585 [details]
review.txt

Comment 13 Remi Collet 2015-08-02 18:06:03 UTC
Everything is OK now.
No blocker


=== APPROVED ===

Comment 14 Shawn Iwinski 2015-08-03 01:52:56 UTC
THANKS for the review!



New Package SCM Request
=======================
Package Name: php-pimple
Short Description: A simple dependency injection container for PHP
Upstream URL: http://pimple.sensiolabs.org/
Owners: siwinski
Branches: f21 f22 epel7
InitialCC:

Comment 15 Shawn Iwinski 2015-08-03 15:21:03 UTC
New Package SCM Request
=======================
Package Name: php-pimple
Short Description: A simple dependency injection container for PHP
Upstream URL: http://pimple.sensiolabs.org/
Owners: siwinski
Branches: f21 f22 f23 epel7
InitialCC:

Comment 16 Gwyn Ciesla 2015-08-04 13:04:02 UTC
Git done (by process-git-requests).

Comment 17 Fedora Update System 2015-08-05 02:17:04 UTC
php-pimple-3.0.1-1.fc21 has been submitted as an update for Fedora 21.
https://admin.fedoraproject.org/updates/php-pimple-3.0.1-1.fc21

Comment 18 Fedora Update System 2015-08-05 02:17:11 UTC
php-pimple-3.0.1-1.fc22 has been submitted as an update for Fedora 22.
https://admin.fedoraproject.org/updates/php-pimple-3.0.1-1.fc22

Comment 19 Fedora Update System 2015-08-05 02:17:18 UTC
php-pimple-3.0.1-1.el7 has been submitted as an update for Fedora EPEL 7.
https://admin.fedoraproject.org/updates/php-pimple-3.0.1-1.el7

Comment 20 Fedora Update System 2015-08-05 02:18:27 UTC
php-pimple-3.0.1-1.fc23 has been submitted as an update for Fedora 23.
https://admin.fedoraproject.org/updates/php-pimple-3.0.1-1.fc23

Comment 21 Fedora Update System 2015-08-10 10:08:28 UTC
php-pimple-3.0.1-1.fc23 has been pushed to the Fedora 23 stable repository.

Comment 22 Fedora Update System 2015-08-13 16:54:01 UTC
php-pimple-3.0.1-1.fc22 has been pushed to the Fedora 22 stable repository.

Comment 23 Fedora Update System 2015-08-13 16:56:18 UTC
php-pimple-3.0.1-1.fc21 has been pushed to the Fedora 21 stable repository.

Comment 24 Fedora Update System 2015-08-22 19:25:21 UTC
php-pimple-3.0.1-1.el7 has been pushed to the Fedora EPEL 7 stable repository. If problems still persist, please make note of it in this bug report.


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