[Bug 1356552] New: Review Request: php-onelogin-php-saml - SAML support for PHP softwares

[Red Hat Bugzilla]

https://bugzilla.redhat.com/show_bug.cgi?id=1356552

Bug ID: 1356552
           Summary: Review Request: php-onelogin-php-saml - SAML support
                    for PHP softwares
           Product: Fedora
           Version: rawhide
         Component: Package Review
          Severity: medium
          Priority: medium
          Assignee: [e-mail]
          Reporter: [e-mail]
        QA Contact: [e-mail]
                CC: [e-mail]

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.fc2...

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

[Bug 1356552] Review Request: php-onelogin-php-saml - SAML support for PHP softwares

[Red Hat Bugzilla]

https://bugzilla.redhat.com/show_bug.cgi?id=1356552

James Hogarth  changed:

What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |[e-mail]
         Depends On|                            |1356081

Referenced Bugs:

https://bugzilla.redhat.com/show_bug.cgi?id=1356081
[Bug 1356081] Please update to the 2.0.0 version and add an autolader

[Bug 1356552] Review Request: php-onelogin-php-saml - SAML support for PHP softwares

[Red Hat Bugzilla]

https://bugzilla.redhat.com/show_bug.cgi?id=1356552
Bug 1356552 depends on bug 1356081, which changed state.

Bug 1356081 Summary: Please update to the 2.0.0 version and add an autolader
https://bugzilla.redhat.com/show_bug.cgi?id=1356081

What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|NEW                         |CLOSED
         Resolution|---                         |WONTFIX

[Bug 1356552] Review Request: php-onelogin-php-saml - SAML support for PHP softwares

[Red Hat Bugzilla]

https://bugzilla.redhat.com/show_bug.cgi?id=1356552

James Hogarth  changed:

What    |Removed                     |Added
----------------------------------------------------------------------------
         Depends On|1356081                     |1356584
                   |                            |(php-robrichards-xmlseclibs
                   |                            |1)

Referenced Bugs:

https://bugzilla.redhat.com/show_bug.cgi?id=1356081
[Bug 1356081] Please update to the 2.0.0 version and add an autolader
https://bugzilla.redhat.com/show_bug.cgi?id=1356584
[Bug 1356584] Review Request: php-robrichards-xmlseclibs1 - A PHP library
for XML Security (version 1)

[Bug 1356552] Review Request: php-onelogin-php-saml - SAML support for PHP softwares

[Red Hat Bugzilla]

https://bugzilla.redhat.com/show_bug.cgi?id=1356552

James Hogarth  changed:

What    |Removed                     |Added
----------------------------------------------------------------------------
              Alias|                            |php-onelogin-php-saml

--- Comment #1 from James Hogarth  ---
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.

[Bug 1356552] Review Request: php-onelogin-php-saml - SAML support for PHP softwares

[Red Hat Bugzilla]

https://bugzilla.redhat.com/show_bug.cgi?id=1356552

James Hogarth  changed:

What    |Removed                     |Added
----------------------------------------------------------------------------
         Whiteboard|                            |NotReady

[Bug 1356552] Review Request: php-onelogin-php-saml - SAML support for PHP softwares

[Red Hat Bugzilla]

https://bugzilla.redhat.com/show_bug.cgi?id=1356552

--- Comment #2 from James Hogarth  ---
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.

[Bug 1356552] Review Request: php-onelogin-php-saml - SAML support for PHP softwares

[Red Hat Bugzilla]

https://bugzilla.redhat.com/show_bug.cgi?id=1356552

James Hogarth  changed:

What    |Removed                     |Added
----------------------------------------------------------------------------
         Depends On|                            |1357459
         Whiteboard|NotReady                    |

--- Comment #3 from James Hogarth  ---
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.

Referenced Bugs:

https://bugzilla.redhat.com/show_bug.cgi?id=1357459
[Bug 1357459] Non-default value in php.ini an undocumented breaking change
in PHP7

[Bug 1356552] Review Request: php-onelogin-php-saml - SAML support for PHP softwares

[Red Hat Bugzilla]

https://bugzilla.redhat.com/show_bug.cgi?id=1356552

James Hogarth  changed:

What    |Removed                     |Added
----------------------------------------------------------------------------
         Depends On|1357459                     |

--- Comment #4 from James Hogarth  ---
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.

Referenced Bugs:

https://bugzilla.redhat.com/show_bug.cgi?id=1357459
[Bug 1357459] Non-default value in php.ini an undocumented breaking change
in PHP7

[Bug 1356552] Review Request: php-onelogin-php-saml - SAML support for PHP softwares

[Red Hat Bugzilla]

https://bugzilla.redhat.com/show_bug.cgi?id=1356552

--- Comment #5 from James Hogarth  ---
Built on a second try ... go figure ... koji must have had a bad moment:

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

[Bug 1356552] Review Request: php-onelogin-php-saml - SAML support for PHP softwares

[Red Hat Bugzilla]

https://bugzilla.redhat.com/show_bug.cgi?id=1356552

--- Comment #6 from James Hogarth  ---
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.fc2...

[Bug 1356552] Review Request: php-onelogin-php-saml - SAML support for PHP softwares

[Red Hat Bugzilla]

https://bugzilla.redhat.com/show_bug.cgi?id=1356552

Remi Collet  changed:

What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|NEW                         |ASSIGNED
           Assignee|[e-mail]    |[e-mail]

--- Comment #7 from Remi Collet  ---
Created attachment 1182086
  --> https://bugzilla.redhat.com/attachment.cgi?id=1182086&action=edit
phpci.log

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

[Bug 1356552] Review Request: php-onelogin-php-saml - SAML support for PHP softwares

[Red Hat Bugzilla]

https://bugzilla.redhat.com/show_bug.cgi?id=1356552

Remi Collet  changed:

What    |Removed                     |Added
----------------------------------------------------------------------------
              Flags|                            |fedora-review?

--- Comment #8 from Remi Collet  ---
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)

[Bug 1356552] Review Request: php-onelogin-php-saml - SAML support for PHP softwares

[Red Hat Bugzilla]

https://bugzilla.redhat.com/show_bug.cgi?id=1356552

--- Comment #9 from Remi Collet  ---
Scratch build: http://koji.fedoraproject.org/koji/taskinfo?taskID=14957389

[Bug 1356552] Review Request: php-onelogin-php-saml - SAML support for PHP softwares

[Red Hat Bugzilla]

https://bugzilla.redhat.com/show_bug.cgi?id=1356552

--- Comment #10 from Remi Collet  ---
Created attachment 1182090
  --> https://bugzilla.redhat.com/attachment.cgi?id=1182090&action=edit
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

[Bug 1356552] Review Request: php-onelogin-php-saml - SAML support for PHP softwares

[Red Hat Bugzilla]

https://bugzilla.redhat.com/show_bug.cgi?id=1356552

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

[Bug 1356552] Review Request: php-onelogin-php-saml - SAML support for PHP softwares

[Red Hat Bugzilla]

https://bugzilla.redhat.com/show_bug.cgi?id=1356552
Bug 1356552 depends on bug 1356584, which changed state.

Bug 1356584 Summary: Review Request: php-robrichards-xmlseclibs1 - A PHP library for XML Security (version 1)
https://bugzilla.redhat.com/show_bug.cgi?id=1356584

What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|ON_QA                       |CLOSED
         Resolution|---                         |ERRATA

[Bug 1356552] Review Request: php-onelogin-php-saml - SAML support for PHP softwares

[Red Hat Bugzilla]

https://bugzilla.redhat.com/show_bug.cgi?id=1356552

--- Comment #12 from James Hogarth  ---
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.fc2...

[Bug 1356552] Review Request: php-onelogin-php-saml - SAML support for PHP softwares

[Red Hat Bugzilla]

https://bugzilla.redhat.com/show_bug.cgi?id=1356552

--- Comment #13 from James Hogarth  ---
Koji scratch build: http://koji.fedoraproject.org/koji/taskinfo?taskID=15012687

[Bug 1356552] Review Request: php-onelogin-php-saml - SAML support for PHP softwares

[Red Hat Bugzilla]

https://bugzilla.redhat.com/show_bug.cgi?id=1356552

--- Comment #14 from James Hogarth  ---
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.fc2...

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

[Bug 1356552] Review Request: php-onelogin-php-saml - SAML support for PHP softwares

[Red Hat Bugzilla]

https://bugzilla.redhat.com/show_bug.cgi?id=1356552

Remi Collet  changed:

What    |Removed                     |Added
----------------------------------------------------------------------------
              Flags|fedora-review?              |fedora-review+

--- Comment #15 from Remi Collet  ---
--- 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/%%7Bgh_owner%7D/%%7Bgh_project%7D
 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  - 2.9.1-3
+- Switch to a single autoloader after feedback
+
+* Mon Jul 25 2016 James Hogarth  - 2.9.1-2
+- Update spec with comments from review
+
 * Wed Jul 20 2016 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 ===

[Bug 1356552] Review Request: php-onelogin-php-saml - SAML support for PHP softwares

[Red Hat Bugzilla]

https://bugzilla.redhat.com/show_bug.cgi?id=1356552

--- Comment #16 from Jon Ciesla  ---
Package request has been approved:
https://admin.fedoraproject.org/pkgdb/package/rpms/php-onelogin-php-saml

[Bug 1356552] Review Request: php-onelogin-php-saml - SAML support for PHP softwares

[Red Hat Bugzilla]

https://bugzilla.redhat.com/show_bug.cgi?id=1356552

--- Comment #17 from Fedora Update System  ---
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

[Bug 1356552] Review Request: php-onelogin-php-saml - SAML support for PHP softwares

[Red Hat Bugzilla]

https://bugzilla.redhat.com/show_bug.cgi?id=1356552

Fedora Update System  changed:

What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|ASSIGNED                    |MODIFIED

[Bug 1356552] Review Request: php-onelogin-php-saml - SAML support for PHP softwares

[Red Hat Bugzilla]

https://bugzilla.redhat.com/show_bug.cgi?id=1356552

--- Comment #19 from Fedora Update System  ---
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

[Bug 1356552] Review Request: php-onelogin-php-saml - SAML support for PHP softwares

[Red Hat Bugzilla]

https://bugzilla.redhat.com/show_bug.cgi?id=1356552

--- Comment #18 from Fedora Update System  ---
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

[Bug 1356552] Review Request: php-onelogin-php-saml - SAML support for PHP softwares

[Red Hat Bugzilla]

https://bugzilla.redhat.com/show_bug.cgi?id=1356552

Fedora Update System  changed:

What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|MODIFIED                    |ON_QA

--- Comment #20 from Fedora Update System  ---
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

[Bug 1356552] Review Request: php-onelogin-php-saml - SAML support for PHP softwares

[Red Hat Bugzilla]

https://bugzilla.redhat.com/show_bug.cgi?id=1356552

--- Comment #21 from Fedora Update System  ---
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

[Bug 1356552] Review Request: php-onelogin-php-saml - SAML support for PHP softwares

[Red Hat Bugzilla]

https://bugzilla.redhat.com/show_bug.cgi?id=1356552

--- Comment #22 from Fedora Update System  ---
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

[Bug 1356552] Review Request: php-onelogin-php-saml - SAML support for PHP softwares

[Red Hat Bugzilla]

https://bugzilla.redhat.com/show_bug.cgi?id=1356552

James Hogarth  changed:

What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|ON_QA                       |CLOSED
         Resolution|---                         |RAWHIDE
        Last Closed|                            |2016-08-01 10:00:23

[Bug 1356552] Review Request: php-onelogin-php-saml - SAML support for PHP softwares

[Red Hat Bugzilla]

https://bugzilla.redhat.com/show_bug.cgi?id=1356552

--- Comment #23 from Fedora Update System  ---
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.

[Bug 1356552] Review Request: php-onelogin-php-saml - SAML support for PHP softwares

[Red Hat Bugzilla]

https://bugzilla.redhat.com/show_bug.cgi?id=1356552

--- Comment #24 from Fedora Update System  ---
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.

[Bug 1356552] Review Request: php-onelogin-php-saml - SAML support for PHP softwares

[Red Hat Bugzilla]

https://bugzilla.redhat.com/show_bug.cgi?id=1356552

--- Comment #25 from Fedora Update System  ---
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.