Bug 823046 - Review Request: php-symfony2-DependencyInjection - Symfony2 DependencyInjection Component
Review Request: php-symfony2-DependencyInjection - Symfony2 DependencyInjecti...
Status: CLOSED ERRATA
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
unspecified Severity medium
: ---
: ---
Assigned To: Remi Collet
Fedora Extras Quality Assurance
:
Depends On: php-channel-symfony2 817303 823042
Blocks: 823073
  Show dependency treegraph
 
Reported: 2012-05-18 17:25 EDT by Shawn Iwinski
Modified: 2012-07-03 15:32 EDT (History)
3 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2012-07-02 18:28:35 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
fedora: fedora‑review+
limburgher: fedora‑cvs+


Attachments (Terms of Use)
php-symfony2-DependencyInjection-review.txt (5.97 KB, text/plain)
2012-06-06 13:11 EDT, Remi Collet
fedora: review?
Details

  None (edit)
Description Shawn Iwinski 2012-05-18 17:25:40 EDT
Spec URL:
http://people.redhat.com/siwinski/rpmbuild/SPECS/php-symfony2-DependencyInjection.spec

SRPM URL:
http://people.redhat.com/siwinski/rpmbuild/SRPMS/php-symfony2-DependencyInjection-2.0.14-1.fc16.src.rpm

Description:
The Dependency Injection component allows you to standardize and centralize the
way objects are constructed in your application.

For an introduction to Dependency Injection and service containers see
Service Container (http://symfony.com/doc/current/book/service_container.html).
Comment 1 Shawn Iwinski 2012-05-20 13:47:15 EDT
Updates per comments in bug 823043

- Removed BuildRoot
- Changed php require to php-common
- Added the following requires based on phpci results:
  php-ctype, php-dom, php-libxml, php-pcre, php-spl, php-simplexml
- Removed %defattr from %files section

SPEC URL:
http://people.redhat.com/siwinski/rpmbuild/SPECS/php-symfony2-DependencyInjection.spec

SRPM URL:
http://people.redhat.com/siwinski/rpmbuild/SRPMS/php-symfony2-DependencyInjection-2.0.14-2.fc16.src.rpm
Comment 3 Shawn Iwinski 2012-05-31 13:09:38 EDT
Updated to upstream version 2.0.15 & updates per bug #817303

- Removed "BuildRequires: php-pear >= 1:1.4.9-1.2"
- Updated %prep section
- Removed cleaning buildroot from %install section
- Removed documentation move from %install section (fixed upstream)
- Removed %clean section
- Updated %doc in %files section

SPEC URL: http://people.redhat.com/siwinski/rpmbuild/SPECS/php-symfony2-DependencyInjection.spec

SRPM URL: http://people.redhat.com/siwinski/rpmbuild/SRPMS/php-symfony2-DependencyInjection-2.0.15-1.fc16.src.rpm
Comment 4 Remi Collet 2012-06-06 13:11:24 EDT
Created attachment 589962 [details]
php-symfony2-DependencyInjection-review.txt

Generated by fedora-review 0.1.3
Comment 5 Remi Collet 2012-06-06 13:13:10 EDT
SHOULD: From package.xml, there are 2 optionnal deps.
php-pear(pear.symfony.com/Config)
php-pear(pear.symfony.com/Yaml)


It's a packager choice to make this (mandatory) Requires.

If you choose to add this requires, you can also remove
%dir %{pear_phpdir}/Symfony/Component
%dir %{pear_phpdir}/Symfony


No blocker.

=== APPROVED ===
Comment 6 Shawn Iwinski 2012-06-07 17:43:11 EDT
New Package SCM Request
=======================
Package Name: php-symfony2-DependencyInjection
Short Description: Symfony2 DependencyInjection Component
Owners: siwinski
Branches: f16 f17 el6
InitialCC:
Comment 7 Gwyn Ciesla 2012-06-08 08:42:55 EDT
Git done (by process-git-requests).
Comment 8 Remi Collet 2012-06-10 05:54:33 EDT
So for late error detection

DependencyInjection/Loader/schema/dic/services/services-1.0.xsd should be installed as role="php", not as role="doc" as this path is referred in the code

In DependencyInjection/Loader/XmlFileLoader.php

   $schemaLocations = array('http://symfony.com/schema/dic/services' 
   => str_replace('\\', '/', __DIR__.'/schema/dic/services/services-1.0.xsd'));


Like for "Locale" this should be reported upstream, and role="data" is probably a better solution.
Comment 9 Shawn Iwinski 2012-06-10 18:45:12 EDT
(In reply to comment #8)
> So for late error detection
> 
> DependencyInjection/Loader/schema/dic/services/services-1.0.xsd should be
> installed as role="php", not as role="doc" as this path is referred in the
> code
> 
> In DependencyInjection/Loader/XmlFileLoader.php
> 
>    $schemaLocations = array('http://symfony.com/schema/dic/services' 
>    => str_replace('\\', '/',
> __DIR__.'/schema/dic/services/services-1.0.xsd'));
> 
> 
> Like for "Locale" this should be reported upstream, and role="data" is
> probably a better solution.

Thanks for finding this Remi.  I will make sure to fix the issue before proceeding.

I will get upstream to fix the package.xml role for the *.xsd file.  It will take a code change to allow for role="data", but I will work with upstream to fix the issue.  If role="data" will take some time to implement, is it fine to revert to role="php" for now and plan on role="data" for a future enhancement?

I looked through all the other php-symfony2-* packages and it appears this same error exists in version 2.0.15 for the Routing, Translation, and Validator packages as well.  I will update those reviews with this information and work with upstream to fix.

Since these fixes upstream will require a new release (probably 2.0.16), may I just fix the package.xml in the spec file?
Comment 10 Remi Collet 2012-06-11 00:50:51 EDT
(In reply to comment #9)
> If role="data" will take some time to implement, is it
> fine to revert to role="php" for now and plan on role="data" for a future
> enhancement?

Yes.

> Since these fixes upstream will require a new release (probably 2.0.16), may
> I just fix the package.xml in the spec file?

Yes
Comment 12 Fedora Update System 2012-06-13 01:35:30 EDT
php-symfony2-DependencyInjection-2.0.15-3.fc17 has been submitted as an update for Fedora 17.
https://admin.fedoraproject.org/updates/php-symfony2-DependencyInjection-2.0.15-3.fc17
Comment 13 Fedora Update System 2012-06-13 01:35:39 EDT
php-symfony2-DependencyInjection-2.0.15-3.fc16 has been submitted as an update for Fedora 16.
https://admin.fedoraproject.org/updates/php-symfony2-DependencyInjection-2.0.15-3.fc16
Comment 14 Fedora Update System 2012-06-13 01:35:49 EDT
php-symfony2-DependencyInjection-2.0.15-3.el6 has been submitted as an update for Fedora EPEL 6.
https://admin.fedoraproject.org/updates/php-symfony2-DependencyInjection-2.0.15-3.el6
Comment 15 Fedora Update System 2012-06-14 20:20:23 EDT
php-symfony2-DependencyInjection-2.0.15-3.fc17 has been pushed to the Fedora 17 testing repository.
Comment 16 Fedora Update System 2012-07-02 18:28:35 EDT
php-symfony2-DependencyInjection-2.0.15-3.fc16 has been pushed to the Fedora 16 stable repository.
Comment 17 Fedora Update System 2012-07-02 18:33:21 EDT
php-symfony2-DependencyInjection-2.0.15-3.fc17 has been pushed to the Fedora 17 stable repository.
Comment 18 Fedora Update System 2012-07-03 15:32:36 EDT
php-symfony2-DependencyInjection-2.0.15-3.el6 has been pushed to the Fedora EPEL 6 stable repository.

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