Bug 1356587 (php-simplesamlphp-saml2)

Summary: Review Request: php-simplesamlphp-saml2 - SAML2 PHP library from SimpleSAMLphp
Product: [Fedora] Fedora Reporter: Shawn Iwinski <shawn>
Component: Package ReviewAssignee: Remi Collet <fedora>
Status: CLOSED ERRATA QA Contact: Fedora Extras Quality Assurance <extras-qa>
Severity: medium Docs Contact:
Priority: medium    
Version: rawhideCC: fedora, package-review
Target Milestone: ---Flags: fedora: fedora-review+
Target Release: ---   
Hardware: All   
OS: Linux   
Whiteboard:
Fixed In Version: Doc Type: If docs needed, set a value
Doc Text:
Story Points: ---
Clone Of: Environment:
Last Closed: 2016-08-09 04:50:45 UTC Type: ---
Regression: --- Mount Type: ---
Documentation: --- CRM:
Verified Versions: Category: ---
oVirt Team: --- RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: --- Target Upstream Version:
Embargoed:
Bug Depends On: 1356569    
Bug Blocks:    
Attachments:
Description Flags
phpci.log
none
review.txt none

Description Shawn Iwinski 2016-07-14 12:32:38 UTC 
Spec URL: https://raw.githubusercontent.com/siwinski/rpms/a4794fbacbe6a03f5a9fbabdd1af53f5820648a1/php-simplesamlphp-saml2/php-simplesamlphp-saml2.spec

SRPM URL: https://siwinski.fedorapeople.org/SRPMS/php-simplesamlphp-saml2-2.2-1.fc24.src.rpm

Description:
A PHP library for SAML2 related functionality. Extracted from SimpleSAMLphp [1],
used by OpenConext [2]. This library started as a collaboration between
UNINETT [3] and SURFnet [4] but everyone is invited to contribute.

Autoloader: %{phpdir}/SAML2/autoload.php

[1] https://www.simplesamlphp.org/
[2] https://www.openconext.org/
[3] https://www.uninett.no/
[4] https://www.surfnet.nl/


Fedora Account System Username: siwinski



Copr build: https://copr.fedorainfracloud.org/coprs/siwinski/simplesamlphp/package/php-simplesamlphp-saml2/
Comment 1 Remi Collet 2016-07-29 13:32:00 UTC 
Created attachment 1185557 [details]
phpci.log

phpCompatInfo version 5.0.1 DB version 1.11.0 built Jul 26 2016 06:33:42 CEST static analyze results
Comment 2 Remi Collet 2016-07-29 13:32:39 UTC 
Created attachment 1185558 [details]
review.txt

Generated by fedora-review 0.6.1 (f03e4e7) last change: 2016-05-02
Command line :/usr/bin/fedora-review -b 1356587
Buildroot used: fedora-rawhide-x86_64
Comment 3 Remi Collet 2016-07-29 13:37:26 UTC 
Everything looks ok (from packaging PoV), no Blockers


Only one clarification:

IIUC, _autoload.php is designed for compatibility with non-namespaced version 1 of the library.

So application using this library probably don't need it.

Instead of including _autoload.php (without NS) from autoload.php (SAML2 NS) shouldn't it be better to do the opposite ?

application using v1 will include _autoload.php, thus will get a working autoloader without NS (and SAML2 NS required by class alias).

application using v2 will include autoload.php, this will get a working autoloader "only" for SAML2 NS.

What do you think ?
Comment 4 Shawn Iwinski 2016-07-29 14:30:30 UTC 
(In reply to Remi Collet from comment #3)
> Everything looks ok (from packaging PoV), no Blockers
> 
> 
> Only one clarification:
> 
> IIUC, _autoload.php is designed for compatibility with non-namespaced
> version 1 of the library.
> 
> So application using this library probably don't need it.
> 
> Instead of including _autoload.php (without NS) from autoload.php (SAML2 NS)
> shouldn't it be better to do the opposite ?
> 
> application using v1 will include _autoload.php, thus will get a working
> autoloader without NS (and SAML2 NS required by class alias).
> 
> application using v2 will include autoload.php, this will get a working
> autoloader "only" for SAML2 NS.
> 
> What do you think ?

I don't like that `_autoload.php` states "Temporary autoloader".  I do not want to be responsible for making sure that file is correct and v1 compatibility always works if just packaging v2.  I would much rather have the two separate pkgs and just keep them updated to upstream's separate releases.  If you look at upstream releases [1], 1.9 was released after 2.2.  Even though right now v2 might be suitable as a drop-in replacement for v1, I'd rather not be responsible for that.

[1] https://github.com/simplesamlphp/saml2/releases
Comment 5 Remi Collet 2016-07-29 14:39:15 UTC 
> I would much rather have the two separate pkgs

Ok, so just don't package this _autoload.php ;)
Comment 6 Shawn Iwinski 2016-07-29 15:38:54 UTC 
(In reply to Remi Collet from comment #5)
> > I would much rather have the two separate pkgs
> 
> Ok, so just don't package this _autoload.php ;)

OK, I'll remove it after initial import



THANKS for the review!  SCM request submitted via pkgdb.
Comment 7 Gwyn Ciesla 2016-07-29 16:35:10 UTC 
Package request has been approved: https://admin.fedoraproject.org/pkgdb/package/rpms/php-simplesamlphp-saml2
Comment 8 Fedora Update System 2016-07-31 04:55:47 UTC 
php-simplesamlphp-saml2-2.2-2.el6 has been submitted as an update to Fedora EPEL 6. https://bodhi.fedoraproject.org/updates/FEDORA-EPEL-2016-9c4a3a8b05
Comment 9 Fedora Update System 2016-07-31 04:55:54 UTC 
php-simplesamlphp-saml2-2.2-2.fc24 has been submitted as an update to Fedora 24. https://bodhi.fedoraproject.org/updates/FEDORA-2016-9a44649dc6
Comment 10 Fedora Update System 2016-07-31 04:55:58 UTC 
php-simplesamlphp-saml2-2.2-2.fc23 has been submitted as an update to Fedora 23. https://bodhi.fedoraproject.org/updates/FEDORA-2016-6288c4728e
Comment 11 Fedora Update System 2016-07-31 04:56:02 UTC 
php-simplesamlphp-saml2-2.2-2.el7 has been submitted as an update to Fedora EPEL 7. https://bodhi.fedoraproject.org/updates/FEDORA-EPEL-2016-6e6d8a3b98
Comment 12 Fedora Update System 2016-08-01 19:16:55 UTC 
php-simplesamlphp-saml2-2.2-2.el6 has been pushed to the Fedora EPEL 6 testing repository. If problems still persist, please make note of it in this bug report.
See https://fedoraproject.org/wiki/QA:Updates_Testing for
instructions on how to install test updates.
You can provide feedback for this update here: https://bodhi.fedoraproject.org/updates/FEDORA-EPEL-2016-9c4a3a8b05
Comment 13 Fedora Update System 2016-08-01 19:48:06 UTC 
php-simplesamlphp-saml2-2.2-2.el7 has been pushed to the Fedora EPEL 7 testing repository. If problems still persist, please make note of it in this bug report.
See https://fedoraproject.org/wiki/QA:Updates_Testing for
instructions on how to install test updates.
You can provide feedback for this update here: https://bodhi.fedoraproject.org/updates/FEDORA-EPEL-2016-6e6d8a3b98
Comment 14 Fedora Update System 2016-08-01 20:54:32 UTC 
php-simplesamlphp-saml2-2.2-2.fc23 has been pushed to the Fedora 23 testing repository. If problems still persist, please make note of it in this bug report.
See https://fedoraproject.org/wiki/QA:Updates_Testing for
instructions on how to install test updates.
You can provide feedback for this update here: https://bodhi.fedoraproject.org/updates/FEDORA-2016-6288c4728e
Comment 15 Fedora Update System 2016-08-01 20:57:45 UTC 
php-simplesamlphp-saml2-2.2-2.fc24 has been pushed to the Fedora 24 testing repository. If problems still persist, please make note of it in this bug report.
See https://fedoraproject.org/wiki/QA:Updates_Testing for
instructions on how to install test updates.
You can provide feedback for this update here: https://bodhi.fedoraproject.org/updates/FEDORA-2016-9a44649dc6
Comment 16 Fedora Update System 2016-08-09 04:50:42 UTC 
php-simplesamlphp-saml2-2.2-2.fc24 has been pushed to the Fedora 24 stable repository. If problems still persist, please make note of it in this bug report.
Comment 17 Fedora Update System 2016-08-09 07:25:06 UTC 
php-simplesamlphp-saml2-2.2-2.fc23 has been pushed to the Fedora 23 stable repository. If problems still persist, please make note of it in this bug report.
Comment 18 Fedora Update System 2016-08-16 19:48:01 UTC 
php-simplesamlphp-saml2-2.2-2.el7 has been pushed to the Fedora EPEL 7 stable repository. If problems still persist, please make note of it in this bug report.
Comment 19 Fedora Update System 2016-08-16 19:49:06 UTC 
php-simplesamlphp-saml2-2.2-2.el6 has been pushed to the Fedora EPEL 6 stable repository. If problems still persist, please make note of it in this bug report.