Bug 823071

Summary: Review Request: php-symfony2-Form - Symfony2 Form Component
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: unspecified    
Version: rawhideCC: christof, fedora, notting, package-review
Target Milestone: ---Flags: fedora: fedora-review+
gwync: fedora-cvs+
Target Release: ---   
Hardware: All   
OS: Linux   
Whiteboard:
Fixed In Version: Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of: Environment:
Last Closed: 2012-07-12 18:54:15 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: 814994, 823050, 823054, 823056, 823066, 859271    
Bug Blocks: 823075    
Attachments:
Description Flags
php-symfony2-Form-review.txt none

Comment 1 Remi Collet 2012-05-19 06:50:18 UTC
Just a quick notes

- LICENSE and README file are installed in /usr/share/pear.

Probably a good idea to ask upstream, this files should be tagged as "doc" and
so installed in /usr/share/doc/pear (and avoid to be duplicated in the package)

- Extension dependencies

# phpci print --recursive --report extension Symfony
-------------------------------------------------------------------------------
PHP COMPAT INFO EXTENSION SUMMARY
-------------------------------------------------------------------------------
  EXTENSION                                        PECL   VERSION         COUNT
-------------------------------------------------------------------------------
  Core                                                    4.0.0             309
  SPL                                                0.2  5.0.0               5
  ctype                                                   4.0.4               3
  date                                                    4.0.0              10
  intl                                             1.1.0  5.2.4              12
  pcre                                                    4.0.0              13
  session                                                 4.0.0               3
  standard                                                4.0.0             689
-------------------------------------------------------------------------------

So need to requires php-intl

- Other classes dependencies

From package.xml => ok
Requires:       php-pear((%{pear_channel}/EventDispatcher)
Requires:       php-pear((%{pear_channel}/Validator)
Requires:       php-pear((%{pear_channel}/Locale)

And optionnaly (your decision) 
Requires:       php-pear((%{pear_channel}/HttpFoundation)

- directory ownership

%dir %{pear_phpdir}/Symfony/Component
%dir %{pear_phpdir}/Symfony

You don't need to own this directories which are already owned by required packages.

Comment 2 Shawn Iwinski 2012-05-19 23:26:24 UTC
(In reply to comment #1)
> Just a quick notes
> 
> - LICENSE and README file are installed in /usr/share/pear.
> 
> Probably a good idea to ask upstream, this files should be tagged as "doc"
> and
> so installed in /usr/share/doc/pear (and avoid to be duplicated in the
> package)

I will request the change upstream.  If they refuse or take a very long to make the change, is this a blocker?  Is it acceptable to use a sed command in the spec file to modify the package.xml file to change the roles of those files to doc?


> - Extension dependencies
> 
> # phpci print --recursive --report extension Symfony
> -----------------------------------------------------------------------------
> --
> PHP COMPAT INFO EXTENSION SUMMARY
> -----------------------------------------------------------------------------
> --
>   EXTENSION                                        PECL   VERSION        
> COUNT
> -----------------------------------------------------------------------------
> --
>   Core                                                    4.0.0            
> 309
>   SPL                                                0.2  5.0.0             
> 5
>   ctype                                                   4.0.4             
> 3
>   date                                                    4.0.0             
> 10
>   intl                                             1.1.0  5.2.4             
> 12
>   pcre                                                    4.0.0             
> 13
>   session                                                 4.0.0             
> 3
>   standard                                                4.0.0            
> 689
> -----------------------------------------------------------------------------
> --
> 
> So need to requires php-intl

I'm getting different phpci results and do not see the intl extension listed:

$ uname -a
Linux siwinski-fedora 3.3.2-6.fc16.x86_64 #1 SMP Sat Apr 21 12:43:20 UTC 2012 x86_64 x86_64 x86_64 GNU/Linux
$ phpci --version
phpci version 2.3.0.
$ phpci print --recursive --report extension Form-2.0.14
121 / 121 [+++++++++++++++++++++++++++++++++++++++++++++++++++++++++>] 100.00%
BASE: /home/siwinski/rpmbuild/SOURCES/Form-2.0.14
-------------------------------------------------------------------------------
PHP COMPAT INFO EXTENSION SUMMARY
-------------------------------------------------------------------------------
  EXTENSION                                        PECL   VERSION         COUNT
-------------------------------------------------------------------------------
  Core                                                    4.0.0             309
  SPL                                                0.2  5.0.0               5
  ctype                                                   4.0.4               3
  date                                                    4.0.0              10
  pcre                                                    4.0.0              13
  session                                                 4.0.0               3
  standard                                                4.0.0             689
-------------------------------------------------------------------------------
A TOTAL OF 7 EXTENSION(S) WERE FOUND
REQUIRED PHP 5.0.0 (MIN) 
-------------------------------------------------------------------------------
Time: 7 seconds, Memory: 20.75Mb
-------------------------------------------------------------------------------


> - Other classes dependencies
> 
> From package.xml => ok
> Requires:       php-pear((%{pear_channel}/EventDispatcher)
> Requires:       php-pear((%{pear_channel}/Validator)
> Requires:       php-pear((%{pear_channel}/Locale)
> 
> And optionnaly (your decision) 
> Requires:       php-pear((%{pear_channel}/HttpFoundation)

I would prefer to let end-users decide if they want to use optional PEAR packages


> - directory ownership
> 
> %dir %{pear_phpdir}/Symfony/Component
> %dir %{pear_phpdir}/Symfony
> 
> You don't need to own this directories which are already owned by required
> packages.

I was just keeping the spec files consistent.  May I just comment out the those 2 lines and add a note as to why they are commented out, or do you want me to delete them?

Comment 3 Shawn Iwinski 2012-05-20 22:15:20 UTC
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-date, php-pcre, php-session, php-spl
- Removed %defattr from %files section
- Removed ownership for directories already owned by required packages

Update per https://bugzilla.redhat.com/show_bug.cgi?id=823041#c5

- Moved documentation to correct location

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

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

Comment 4 Shawn Iwinski 2012-05-20 22:19:46 UTC
(In reply to comment #2)
> (In reply to comment #1)
> > - Extension dependencies
> > 
> > # phpci print --recursive --report extension Symfony
> > -----------------------------------------------------------------------------
> > --
> > PHP COMPAT INFO EXTENSION SUMMARY
> > -----------------------------------------------------------------------------
> > --
> >   EXTENSION                                        PECL   VERSION        
> > COUNT
> > -----------------------------------------------------------------------------
> > --
> >   Core                                                    4.0.0            
> > 309
> >   SPL                                                0.2  5.0.0             
> > 5
> >   ctype                                                   4.0.4             
> > 3
> >   date                                                    4.0.0             
> > 10
> >   intl                                             1.1.0  5.2.4             
> > 12
> >   pcre                                                    4.0.0             
> > 13
> >   session                                                 4.0.0             
> > 3
> >   standard                                                4.0.0            
> > 689
> > -----------------------------------------------------------------------------
> > --
> > 
> > So need to requires php-intl
> 
> I'm getting different phpci results and do not see the intl extension listed:
> 
> $ uname -a
> Linux siwinski-fedora 3.3.2-6.fc16.x86_64 #1 SMP Sat Apr 21 12:43:20 UTC
> 2012 x86_64 x86_64 x86_64 GNU/Linux
> $ phpci --version
> phpci version 2.3.0.
> $ phpci print --recursive --report extension Form-2.0.14
> 121 / 121 [+++++++++++++++++++++++++++++++++++++++++++++++++++++++++>]
> 100.00%
> BASE: /home/siwinski/rpmbuild/SOURCES/Form-2.0.14
> -----------------------------------------------------------------------------
> --
> PHP COMPAT INFO EXTENSION SUMMARY
> -----------------------------------------------------------------------------
> --
>   EXTENSION                                        PECL   VERSION        
> COUNT
> -----------------------------------------------------------------------------
> --
>   Core                                                    4.0.0            
> 309
>   SPL                                                0.2  5.0.0             
> 5
>   ctype                                                   4.0.4             
> 3
>   date                                                    4.0.0             
> 10
>   pcre                                                    4.0.0             
> 13
>   session                                                 4.0.0             
> 3
>   standard                                                4.0.0            
> 689
> -----------------------------------------------------------------------------
> --
> A TOTAL OF 7 EXTENSION(S) WERE FOUND
> REQUIRED PHP 5.0.0 (MIN) 
> -----------------------------------------------------------------------------
> --
> Time: 7 seconds, Memory: 20.75Mb
> -----------------------------------------------------------------------------
> --

Remi -- Any idea about the phpci difference with the intl extension?

Comment 5 Remi Collet 2012-05-21 06:01:14 UTC
> Remi -- Any idea about the phpci difference with the intl extension?

phpci only reports about "installed" extensions.
This means : you need to install all extensions to have them detect : yes this is awfull... :(

Another solution is to create a /etc/pear/PHP_CompatInfo/phpcompatinfo.xml 

From : file:///usr/share/doc/pear/PHP_CompatInfo/docs/phpci-book.html
> The XML configuration above corresponds to the default behaviour of the phpcli tool.
> reference
>    Data dictionnary reference name. Defaults to PHP5 for all PHP4 and PHP5 components depending of your extensions loaded.

Comment 6 Remi Collet 2012-05-21 08:40:58 UTC
FYI: I have add a feature request on phpci to have an new reference "ALL" to avoid such detection problem.
https://github.com/llaville/php-compat-info/pull/36

Comment 8 Shawn Iwinski 2012-05-31 17:19:38 UTC
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-Form.spec

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

Comment 9 Shawn Iwinski 2012-06-13 06:33:57 UTC
Added optional require

- Added php-pear(%{pear_channel}/HttpFoundation) require

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

SRPM URL: http://people.redhat.com/~siwinski/rpmbuild/SRPMS/php-symfony2-Form-2.0.15-2.fc17.src.rpm

Comment 10 Remi Collet 2012-06-24 15:57:23 UTC
Can you please check the role for
Symfony/Component/Form/Resources/config/validation.xml

I can't find any "direct" use of this file in the code, but role="doc" seems strange...

Comment 11 Shawn Iwinski 2012-06-25 03:44:10 UTC
(In reply to comment #10)
> Can you please check the role for
> Symfony/Component/Form/Resources/config/validation.xml
> 
> I can't find any "direct" use of this file in the code, but role="doc" seems
> strange...

This appears to be the same type issue with the XSD files in some of the other php-symfony2-* packages although I cannot find any code in this package that references that file either.  I could ask upstream if we're missing something, but perhaps I could just change it's role to "php" (like I did for those XSD files) without acknowledgment from upstream as it seems this version (2.0.15) may be the last of the 2.0.x series as they already have betas out for 2.1.  Recommendation?

Comment 12 Remi Collet 2012-06-25 04:39:05 UTC
(In reply to comment #11)
> Recommendation?

If needed and set as "doc", the package will not work.
If fixed to "php", and not used, the package will work.

So
1/ fix the role, 
2/ ask confirmation to upstream, as this need to be fixed in next 2.1

Comment 13 Shawn Iwinski 2012-06-30 03:49:32 UTC
Update per comment #10 and comment #12

- Changed PEAR role of Symfony/Component/Form/Resources/config/validation.xml
(Note: This has been fixed in upstream version 2.1.0 BETA1)

SPEC URL: http://siwinski.fedorapeople.org/rpmbuild/SPECS/php-symfony2-Form.spec

SRPM URL: http://siwinski.fedorapeople.org/rpmbuild/RPMS/noarch/php-symfony2-Form-2.0.15-3.fc17.noarch.rpm

Comment 14 Remi Collet 2012-06-30 04:45:10 UTC
Created attachment 595409 [details]
php-symfony2-Form-review.txt

Generated by fedora-review 0.1.3

Comment 15 Remi Collet 2012-06-30 04:46:55 UTC
in Form-2.0.15/Symfony/Component/Form/Extension/DependencyInjection/DependencyInjectionExtension.php

use Symfony\Component\DependencyInjection\ContainerInterface;

so I recommend to add this in the optional requires, and see with upstream to add this in the package.xml


No blocker, so

==== APPROVED ====

Comment 16 Shawn Iwinski 2012-06-30 04:59:37 UTC
New Package SCM Request
=======================
Package Name: php-symfony2-Form
Short Description: Symfony2 Form Component
Owners: siwinski
Branches: f16 f17 el6
InitialCC:

Comment 17 Gwyn Ciesla 2012-07-01 22:42:10 UTC
Git done (by process-git-requests).

Comment 18 Fedora Update System 2012-07-02 20:08:32 UTC
php-symfony2-Form-2.0.15-4.el6 has been submitted as an update for Fedora EPEL 6.
https://admin.fedoraproject.org/updates/php-symfony2-Form-2.0.15-4.el6

Comment 19 Fedora Update System 2012-07-02 20:08:43 UTC
php-symfony2-Form-2.0.15-4.fc17 has been submitted as an update for Fedora 17.
https://admin.fedoraproject.org/updates/php-symfony2-Form-2.0.15-4.fc17

Comment 20 Fedora Update System 2012-07-02 20:08:52 UTC
php-symfony2-Form-2.0.15-4.fc16 has been submitted as an update for Fedora 16.
https://admin.fedoraproject.org/updates/php-symfony2-Form-2.0.15-4.fc16

Comment 21 Fedora Update System 2012-07-03 15:53:22 UTC
php-symfony2-Form-2.0.15-4.fc16 has been pushed to the Fedora 16 testing repository.

Comment 22 Fedora Update System 2012-07-12 18:54:15 UTC
php-symfony2-Form-2.0.15-4.fc17 has been pushed to the Fedora 17 stable repository.

Comment 23 Fedora Update System 2012-07-12 19:00:11 UTC
php-symfony2-Form-2.0.15-4.fc16 has been pushed to the Fedora 16 stable repository.

Comment 24 Fedora Update System 2012-07-19 22:34:47 UTC
php-symfony2-Form-2.0.15-4.el6 has been pushed to the Fedora EPEL 6 stable repository.