Note: This is a public test instance of Red Hat Bugzilla. The data contained within is a snapshot of the live data so any changes you make will not be reflected in the production Bugzilla. Email is disabled so feel free to test any aspect of the site that you want. File any problems you find or give feedback at bugzilla.redhat.com.

Bug 1356552 (php-onelogin-php-saml)

Summary: Review Request: php-onelogin-php-saml - SAML support for PHP softwares
Product: [Fedora] Fedora Reporter: James Hogarth <james.hogarth>
Component: Package ReviewAssignee: Remi Collet <fedora>
Status: CLOSED RAWHIDE 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-01 14:00:23 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: 1356584    
Bug Blocks:    
Attachments:
Description Flags
phpci.log
none
review.txt none

Description James Hogarth 2016-07-14 11:16:21 UTC
Spec URL: https://jhogarth.fedorapeople.org/php-saml/php-onelogin-php-saml.spec
SRPM URL: https://jhogarth.fedorapeople.org/php-saml/php-onelogin-php-saml-2.9.0-1.fc24.src.rpm

Description: OneLogin's SAML PHP toolkit let you build a SP (Service Provider) over your PHP application and connect it to any IdP (Identity Provider).

SAML is a standardised authentication mechanism:
https://en.wikipedia.org/wiki/Security_Assertion_Markup_Language



Fedora Account System Username: jhogarth

Comment 1 James Hogarth 2016-07-14 14:02:10 UTC
After discussion with siwiniski changing the requires to the proposed v1 library as despite it looking like it has a v2 requirement ... it really doesn't as that adds only adds namespacing that the embedded xmlseclibs explicitly removes.

Comment 2 James Hogarth 2016-07-15 14:36:04 UTC
Marked as NotReady as it doesn't pass tests in rawhide, though it does in F24.

Looks a difference in the assert() behaviour change from PHP7 is causing it, though the upstream defaults claim that they behave like PHP5.

I'll remove the whiteboard marker when the issue is resolved.

Comment 3 James Hogarth 2016-07-18 09:39:44 UTC
The Fedora PHP build has a zend.assertions that differs from the upstream default which is what is breaking the build/tests in Rawhide.

Removing the NotReady now it's understood why this is the case.

It can be built and tested on F24 in the meanwhile.

Comment 4 James Hogarth 2016-07-18 14:46:26 UTC
Runtime code patched (and PR issued upstream) and after discussion with Remi phpunit being called with both development settings (so assert() related stuff gets tested once) and with system settings (to reflect actual use when assert() gets compiled out with production settings).

Removing the blocker on the zend.assertions php ini setting as that's no longer relevant to this package.

Note that the Rawhide build in koji for some reason: http://koji.fedoraproject.org/koji/taskinfo?taskID=14934573

It did however work in COPR: https://copr.fedorainfracloud.org/coprs/jhogarth/NextCloud/build/390252/

It also built correctly in local mock.

The spec and srpm on #c1 is correct for testing.

Comment 5 James Hogarth 2016-07-18 14:47:27 UTC
Built on a second try ... go figure ... koji must have had a bad moment:

http://koji.fedoraproject.org/koji/taskinfo?taskID=14934630

Comment 6 James Hogarth 2016-07-20 08:50:30 UTC
Upstream has released a new version (2.9.1) with my PR merged.

Spec URL: https://jhogarth.fedorapeople.org/php-saml/php-onelogin-php-saml.spec
SRPM URL: https://jhogarth.fedorapeople.org/php-saml/php-onelogin-php-saml-2.9.1-1.fc25.src.rpm

Comment 7 Remi Collet 2016-07-20 11:44:30 UTC
Created attachment 1182086 [details]
phpci.log

phpCompatInfo version 5.0.1 DB version 1.10.0 built Jul 05 2016 07:54:25 CEST static analyze results

Comment 8 Remi Collet 2016-07-20 11:55:11 UTC
From PHP Guidelines: "PHP extensions must have a Requires on all of the dependent extensions" 

From composer.json

    "require": {
        "php": ">=5.3.2",
        "ext-openssl": "*",
        "ext-dom": "*",
        "ext-mcrypt": "*"
    },

So please add

Requires: php-openssl
Requires: php-dom

From phpcompatinfo analysis (attachement), also add

Requires: php-date
Requires: php-filter
Requires: php-hash
Requires: php-libxml
Requires: php-pcre
Requires: php-session
Requires: php-zlib

Suggests: php-gettext


From PHP Guidelines: "A PSR-0 [1] compliant library would put its PHP files in /usr/share/php/<Vendor Name> "

As provided classes are OneLogin_Saml_* and OneLogin_Saml2_* should be installed in (PSR-0 compliant tree):
/usr/share/php/OneLogin/Saml
/usr/share/php/OneLogin/Saml2

(autoloader in Saml or/and Saml2 directory)

Comment 9 Remi Collet 2016-07-20 11:56:53 UTC
Scratch build: http://koji.fedoraproject.org/koji/taskinfo?taskID=14957389

Comment 10 Remi Collet 2016-07-20 12:06:20 UTC
Created attachment 1182090 [details]
review.txt

Generated by fedora-review 0.6.1 (f03e4e7) last change: 2016-05-02
Command line :/usr/bin/fedora-review -b 1356552
Buildroot used: fedora-rawhide-x86_64

Comment 11 Remi Collet 2016-07-20 12:08:37 UTC
Summary from review.txt
[!]: Requires correct, justified where necessary.
[!]: Package complies to the Packaging Guidelines
[!]: Please add CHANGELOG as %doc

Comment 12 James Hogarth 2016-07-25 11:51:15 UTC
Updated as per comments:

In addition there with the separation of Saml and Saml2 there are two loaders in the Saml directory ... a compat one that uses static loading like that used in the phpunit tests and upstream's compatibility.php and also one using classname based loading such as carried out by the composer based loader in nextcloud's user_saml application.

Spec URL: https://jhogarth.fedorapeople.org/php-saml/php-onelogin-php-saml.spec
SRPM URL: https://jhogarth.fedorapeople.org/php-saml/php-onelogin-php-saml-2.9.1-2.fc25.src.rpm

Comment 13 James Hogarth 2016-07-25 11:54:56 UTC
Koji scratch build: http://koji.fedoraproject.org/koji/taskinfo?taskID=15012687

Comment 14 James Hogarth 2016-07-25 14:30:10 UTC
Following IRC discussion ... reduced to single autoloader but keeping split PSR directories:

Spec URL: https://jhogarth.fedorapeople.org/php-saml/php-onelogin-php-saml.spec
SRPM URL: https://jhogarth.fedorapeople.org/php-saml/php-onelogin-php-saml-2.9.1-3.fc25.src.rpm

koji scratch build: http://koji.fedoraproject.org/koji/taskinfo?taskID=15013330

Comment 15 Remi Collet 2016-07-25 15:13:26 UTC
--- php-onelogin-php-saml.spec.0	2016-07-20 13:37:25.000000000 +0200
+++ php-onelogin-php-saml.spec	2016-07-25 17:07:35.000000000 +0200
@@ -2,18 +2,22 @@
 %global gh_short     %(c=%{gh_commit}; echo ${c:0:7})
 %global gh_owner     onelogin
 %global gh_project   php-saml
+%global php_vendor   OneLogin
 
 %global php_minver 5.3.2
 
 Name:           php-%{gh_owner}-%{gh_project}
 Version:        2.9.1
-Release:        1%{?dist}
+Release:        3%{?dist}
 Summary:        SAML support for PHP
 
 License:        MIT
 URL:            https://github.com/%{gh_owner}/%{gh_project}
 Source0:        %{url}/archive/%{gh_commit}/%{gh_project}-%{version}-%{gh_short}.tar.gz
 
+# Patch the test bootstrap for our autoload.php rather than adjust in %%check to simplify spec
+Patch0:         php-saml-bootstrap-autoloader.patch
+
 BuildArch:      noarch
 
 BuildRequires:  php(language) >= %{php_minver}
@@ -28,19 +32,36 @@
 # From composer.json, "require": {
 #        "php": ">=5.3.2"
 Requires:       php(language) >= %{php_minver}
+Requires: php-openssl
+Requires: php-dom
+
 # From manual unbundling, needs 1.4 contrary to the bundled 2.0 due to namespace issues
 Requires:       php-composer(robrichards/xmlseclibs) >= 1.4.1
 Requires:       php-composer(robrichards/xmlseclibs) < 2.0.0
 
-Provides:       php-composer(%{gh_owner}/%{gh_project}) = %{version}
-# Uses the mcrypt algorithms 
+# From phpci analysis
+Requires: php-date
+Requires: php-filter
+Requires: php-hash
+Requires: php-libxml
+Requires: php-pcre
+Requires: php-session
+Requires: php-zlib
+
+
+Suggests: php-gettext
+
+# Uses the mcrypt algorithms which is a suggests in xmlseclibs
 Requires:       php-mcrypt
 
+Provides:       php-composer(%{gh_owner}/%{gh_project}) = %{version}
+
+
 %description
 OneLogin's SAML PHP toolkit let you build a SP (Service Provider) over 
 your PHP application and connect it to any IdP (Identity Provider).
 
-Autoloader: %{_datadir}/php/%{gh_project}/autoload.php
+Autoloader: %{_datadir}/php/%{php_vendor}/Saml2/autoload.php
 
 
 %prep
@@ -50,27 +71,22 @@
 %build
 rm -rf extlib
 : Generate autoloader
-%{_bindir}/phpab -n --output lib/autoload.php lib
+%{_bindir}/phpab -n --output lib/Saml2/autoload.php lib
 # Append the xmlseclibs requirement not in composer
-cat >> lib/autoload.php <<EOF
+cat >> lib/Saml2/autoload.php <<EOF
 require_once "%{_datadir}/php/robrichards-xmlseclibs/autoload.php";
 EOF
 
 
 %install
-mkdir -p   %{buildroot}%{_datadir}/php/%{gh_project}
-cp -pr lib/* %{buildroot}%{_datadir}/php/%{gh_project}/
+mkdir -p   %{buildroot}%{_datadir}/php/%{php_vendor}
+cp -pr lib/* %{buildroot}%{_datadir}/php/%{php_vendor}/
 
 
 %check
-: Run upstream tests
-# Use our autoloader in tests
-sed -i 's#_toolkit_loader.php#lib/autoload.php#' tests/bootstrap.php
-# Drop the bundled xmlseclibs from being used in bootstrap
-sed -i '/XMLSECLIBS_DIR/d' tests/bootstrap.php
-: Run phpunit tests in dev mode
+: Run upstream phpunit tests in dev mode
 %{_bindir}/php -c %{_docdir}/php/php.ini-development %{_bindir}/phpunit --verbose --debug --bootstrap tests/bootstrap.php --configuration tests/phpunit.xml
-: Run phpunit tests in system settings mode
+: Run upstream phpunit tests in system settings mode
 %{_bindir}/php %{_bindir}/phpunit --verbose --debug --bootstrap tests/bootstrap.php --configuration tests/phpunit.xml
 
 
@@ -79,11 +95,17 @@
 
 %files
 %license LICENSE
-%doc advanced_settings_example.php settings_example.php README.md composer.json
-%{_datadir}/php/%{gh_project}
+%doc advanced_settings_example.php settings_example.php README.md composer.json CHANGELOG
+%{_datadir}/php/%{php_vendor}
 
 
 %changelog
+* Mon Jul 25 2016 James Hogarth <james.hogarth> - 2.9.1-3
+- Switch to a single autoloader after feedback
+
+* Mon Jul 25 2016 James Hogarth <james.hogarth> - 2.9.1-2
+- Update spec with comments from review
+
 * Wed Jul 20 2016 James Hogarth <james.hogarth> - 2.9.1-1
 - update to 2.9.1
 

[x]: Requires correct, justified where necessary.
[x]: Package complies to the Packaging Guidelines
[x]: Please add CHANGELOG as %doc


Everything is now OK.

It seems the "--debug" option is not needed (create lof ot output lines)


=== APPROVED ===

Comment 16 Gwyn Ciesla 2016-07-26 15:03:03 UTC
Package request has been approved: https://admin.fedoraproject.org/pkgdb/package/rpms/php-onelogin-php-saml

Comment 17 Fedora Update System 2016-07-26 20:42:53 UTC
php-onelogin-php-saml-2.9.1-3.fc23 has been submitted as an update to Fedora 23. https://bodhi.fedoraproject.org/updates/FEDORA-2016-de1aca249f

Comment 18 Fedora Update System 2016-07-26 20:43:46 UTC
php-onelogin-php-saml-2.9.1-3.fc24 has been submitted as an update to Fedora 24. https://bodhi.fedoraproject.org/updates/FEDORA-2016-353d77a339

Comment 19 Fedora Update System 2016-07-26 20:44:14 UTC
php-onelogin-php-saml-2.9.1-3.el7 has been submitted as an update to Fedora EPEL 7. https://bodhi.fedoraproject.org/updates/FEDORA-EPEL-2016-81dff2c066

Comment 20 Fedora Update System 2016-07-28 04:08:38 UTC
php-onelogin-php-saml-2.9.1-3.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-81dff2c066

Comment 21 Fedora Update System 2016-07-28 04:18:07 UTC
php-onelogin-php-saml-2.9.1-3.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-de1aca249f

Comment 22 Fedora Update System 2016-07-28 05:59:15 UTC
php-onelogin-php-saml-2.9.1-3.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-353d77a339

Comment 23 Fedora Update System 2016-10-07 06:48:15 UTC
php-onelogin-php-saml-2.9.1-3.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 24 Fedora Update System 2016-10-07 08:24:51 UTC
php-onelogin-php-saml-2.9.1-3.fc24 has been pushed to the Fedora 24 stable repository. If problems still persist, please make note of it in this bug report.

Comment 25 Fedora Update System 2016-10-09 09:23:37 UTC
php-onelogin-php-saml-2.9.1-3.fc23 has been pushed to the Fedora 23 stable repository. If problems still persist, please make note of it in this bug report.