Bug 1414216 - Review Request: php-gettext-gettext - PHP gettext manager
Summary: Review Request: php-gettext-gettext - PHP gettext manager
Keywords:
Status: CLOSED ERRATA
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Remi Collet
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On: 1413434
Blocks:
TreeView+ depends on / blocked
 
Reported: 2017-01-18 04:27 UTC by Randy Barlow
Modified: 2017-04-22 09:22 UTC (History)
4 users (show)

Fixed In Version: php-gettext-gettext-3.5.9-5.fc27
Doc Type: If docs needed, set a value
Doc Text:
Clone Of:
Environment:
Last Closed: 2017-04-17 15:54:29 UTC
Type: ---
fedora: fedora-review+


Attachments (Terms of Use)
phpci-src.log (15.63 KB, text/plain)
2017-03-28 11:57 UTC, Remi Collet
no flags Details
review.txt (7.34 KB, text/plain)
2017-03-28 11:57 UTC, Remi Collet
no flags Details

Description Randy Barlow 2017-01-18 04:27:38 UTC
Spec URL: https://bowlofeggs.fedorapeople.org/php-gettext-gettext.spec
SRPM URL: https://bowlofeggs.fedorapeople.org/php-gettext-gettext-3.5.9-1.fc26.src.rpm
Description: Gettext is a PHP (5.3) library to import/export/edit gettext from PO,                                               
MO, PHP, JS files, etc.
Fedora Account System Username: bowlofeggs

Note that this isn't the latest version of gettext/gettext. I'm packaging this as part of an effort to get Ampache into Fedora[0], and Ampache's composer.json specifies a 3.5.* version of gettext[1]. I filed an issue with Ampache to ask if they could use a newer version[2].

This package depends on another package review I've submitted for gettext/languages, and I've marked this BZ as depending on that one.

[0] https://fedoraproject.org/wiki/Ampache
[1] https://github.com/ampache/ampache/blob/3.8.2/composer.json
[2] https://github.com/ampache/ampache/issues/1467

Comment 1 Remi Collet 2017-01-21 07:23:03 UTC
Mostly same comments than php-gettext-languages

- use a git snapshot to retrieve test suite
- add autoloade (which include php-gettext-languages one)
- run test suite

Comment 2 Randy Barlow 2017-01-22 16:49:50 UTC
Hey Remi!

I ended up adding a require_once onto the existing upstream autoloader - will that work?

As for the tests, this package has four optional dependencies, and two of the tests were failing without them. Since the dependencies are explicitly listed as optional in the composer.json, I opted to skip running them for now. Is that OK by you?

Spec URL: https://bowlofeggs.fedorapeople.org/php-gettext-gettext.spec
SRPM URL: https://bowlofeggs.fedorapeople.org/php-gettext-gettext-3.5.9-2.fc26.src.rpm

Comment 3 Remi Collet 2017-01-23 08:33:58 UTC
(In reply to Randy Barlow from comment #2)

> I ended up adding a require_once onto the existing upstream autoloader -
> will that work?

You need to check in mock to ensure everything is OK.

php-gettext-languages will install in /usr/share/php/Gettext/Languages with its autoloader.
php-gettext-gettext will install in /usr/share/php/Gettext with its autoloader.

As gettext autoloader manage the "Gettext" namespace, il will also manage the Languages sub-namespace, as both are installed in the same directory.

But this WONT work during the build.
(languages in /usr..., gettext in %{buildroot}/usr...)

So you really have to include/require Languages autoloader from the Gettext one.

> As for the tests, this package has four optional dependencies, and two of
> the tests were failing without them. Since the dependencies are explicitly
> listed as optional in the composer.json, I opted to skip running them for
> now. Is that OK by you?

Ok for missing dep.
But twig is available in the repo, so worth to be used (BuildRequires + Suggests)

Which mean the GetText autoloader will need to also load these.

Ex (using fedora autoloader)

    \Fedora\Autoloader\Dependencies::required(array(
        '/usr/share/php/Gettext/Languages/autoloader.php'
    ));
    \Fedora\Autoloader\Dependencies::optional(array(
        '/usr/share/php/Twig/Extensions/autoload.php',
        '/usr/share/php/Twig/autoload.php',
    ));

(which also means the upstream autoloader doesn't really suite our needs)

Comment 4 Remi Collet 2017-01-23 08:37:59 UTC
Fixed example (for twig v1/v2) even, a fully rewritten one (not tested)

    <?php
    require_once '/usr/share/php/Fedora/Autoloader/autoload.php';

    \Fedora\Autoloader\Autoload::addPsr4('Gettext\\', __DIR__);

    \Fedora\Autoloader\Dependencies::required(array(
        '/usr/share/php/Gettext/Languages/autoloader.php'
    ));

    \Fedora\Autoloader\Dependencies::optional(array(
        '/usr/share/php/Twig/Extensions/autoload.php',
        array(
            '/usr/share/php/Twig2/autoload.php',
            '/usr/share/php/Twig/autoload.php',
    )));

Comment 5 Remi Collet 2017-01-23 08:46:22 UTC
Additional notice: in composer.json upstream have

    "autoload": {
        "psr-4": {
            "Gettext\\": "src"
        }
    }

Which means this is PSR-4, and this is what consumer projects will rely on. So, their autoloader is NOT used (probably there for compatibility).

If upstream expect we use their autoloader, they will have

    "autoload": {
        "files": ["src/autoloader.php"],
    }

Perhaps, a way could be to provide both autoloaders:

/usr/share/php/Gettext/autoloader.php 
  => the upstream one, unused

/usr/share/php/Gettext/autoload.php
  => using fedora autoloader (above ex), used in fedora packaging

Comment 6 Randy Barlow 2017-02-02 00:01:36 UTC
Hello Remi!

I've added your example autoloader. When I don't skip the twig related tests, I see this error:


+ phpunit --bootstrap tests/bootstrap.php --filter '^((?!(testBlade)).)*$'
PHPUnit 5.7.5 by Sebastian Bergmann and contributors.

.........E....................................................... 65 / 83 ( 78%)
..................                                                83 / 83 (100%)

Time: 96 ms, Memory: 6.00MB

There was 1 error:

1) Gettext\Tests\AssetsTest::testTwig
Error: Class 'Twig_Loader_String' not found

/home/rbarlow/rpmbuild/BUILDROOT/php-gettext-gettext-3.5.9-3.fc26.x86_64/usr/share/php/Gettext/Gettext/Extractors/Twig.php:38
/home/rbarlow/rpmbuild/BUILDROOT/php-gettext-gettext-3.5.9-3.fc26.x86_64/usr/share/php/Gettext/Gettext/Extractors/Twig.php:26
/home/rbarlow/rpmbuild/BUILDROOT/php-gettext-gettext-3.5.9-3.fc26.x86_64/usr/share/php/Gettext/Gettext/Extractors/Extractor.php:18
/home/rbarlow/rpmbuild/BUILDROOT/php-gettext-gettext-3.5.9-3.fc26.x86_64/usr/share/php/Gettext/Gettext/Translations.php:170
/home/rbarlow/rpmbuild/BUILDROOT/php-gettext-gettext-3.5.9-3.fc26.x86_64/usr/share/php/Gettext/Gettext/Translations.php:149
/home/rbarlow/rpmbuild/BUILD/Gettext-8da2f94538a2250275bb86b431c51e31e256e1e1/tests/AbstractTest.php:42
/home/rbarlow/rpmbuild/BUILD/Gettext-8da2f94538a2250275bb86b431c51e31e256e1e1/tests/AssetsTest.php:376

ERRORS!
Tests: 83, Assertions: 673, Errors: 1.


Perhaps the Twig autoloader doesn't load /usr/share/php/Twig/Loader (which is where this class is)? What do you think?

Here is another build that has your suggested autoload.php but still skips the Twig tests:

Spec URL: https://bowlofeggs.fedorapeople.org/php-gettext-gettext.spec
SRPM URL: https://bowlofeggs.fedorapeople.org/php-gettext-gettext-3.5.9-3.fc26.src.rpm

Comment 7 Remi Collet 2017-02-03 10:30:15 UTC
Twig_Loader_String is only in twig v1 (and deprecated).
So you cannot use Twig 2.

BTW, removing the '/usr/share/php/Twig2/autoload.php' line from the autoloader doesn't really solves the issue:

1) Gettext\Tests\AssetsTest::testTwig
TypeError: Argument 1 passed to Twig_Environment::compileSource() must be an instance of Twig_Source, string given, called in /dev/shm/extras/BUILDROOT/php-gettext-gettext-3.5.9-3.fc25.remi.x86_64/usr/share/php/Gettext/Extractors/Twig.php on line 28

So definitively, this library is not ready for recent twig version (should probably be reported upstream, I know twig is terribly managed, various BC breaks in recent minor versions....)

So, for now, have to drop this optional dep... until fixed by upstream.

(sorry to have suggest you to add it ;)

Comment 8 Randy Barlow 2017-02-13 04:08:12 UTC
Hello Remi! It seems that there is already an upstream issue about Twig 2:

https://github.com/oscarotero/Gettext/issues/137

Here's a build with Twig removed:

Spec URL: https://bowlofeggs.fedorapeople.org/php-gettext-gettext.spec
SRPM URL: https://bowlofeggs.fedorapeople.org/php-gettext-gettext-3.5.9-4.fc26.src.rpm

Comment 9 Remi Collet 2017-03-28 11:57:16 UTC
Created attachment 1266920 [details]
phpci-src.log

phpCompatInfo version 5.0.6 DB version 1.19.0 built Mar 17 2017 06:44:54 CET
run in "src"

Comment 10 Remi Collet 2017-03-28 11:57:47 UTC
Created attachment 1266921 [details]
review.txt

Generated by fedora-review 0.6.1 (f03e4e7) last change: 2016-05-02
Command line :/usr/bin/fedora-review -b 1414216
Buildroot used: fedora-rawhide-x86_64
Active plugins: Generic, PHP, Shell-api

Comment 11 Remi Collet 2017-03-28 11:59:11 UTC
Sorry for long delay (don't why is doesn't appear to me before...)


[!]: Package does not own files or directories owned by other packages.
     Note: Dirs in package are owned also by: /usr/share/php/Gettext(php-
     gettext-languages), which is required.

[!]: Requires correct, justified where necessary.
	See phpci-src.log
	=> missing php-dom ans php-simplexml

Comment 12 Randy Barlow 2017-04-02 21:00:45 UTC
This build fixes the directory ownership and the php dependencies. Thanks!

Spec URL: https://bowlofeggs.fedorapeople.org/php-gettext-gettext.spec
SRPM URL: https://bowlofeggs.fedorapeople.org/php-gettext-gettext-3.5.9-5.fc27.src.rpm

Comment 13 Remi Collet 2017-04-03 06:25:07 UTC
+Requires:   php-dom
+Requires:   php-simplexml

[x]: Requires correct, justified where necessary.

-%{_datadir}/php/Gettext
+%{_datadir}/php/Gettext/*

[x]: Package does not own files or directories owned by other packages.


Blockers fixed.


=== APPROVED ===

Comment 14 Gwyn Ciesla 2017-04-10 12:22:51 UTC
Package request has been approved: https://admin.fedoraproject.org/pkgdb/package/rpms/php-gettext-gettext

Comment 15 Fedora Update System 2017-04-12 22:06:59 UTC
php-gettext-gettext-3.5.9-5.fc26 has been submitted as an update to Fedora 26. https://bodhi.fedoraproject.org/updates/FEDORA-2017-1f428f5e35

Comment 16 Fedora Update System 2017-04-12 22:07:30 UTC
php-gettext-gettext-3.5.9-5.fc25 has been submitted as an update to Fedora 25. https://bodhi.fedoraproject.org/updates/FEDORA-2017-534f555ca4

Comment 17 Fedora Update System 2017-04-12 22:08:04 UTC
php-gettext-gettext-3.5.9-5.fc24 has been submitted as an update to Fedora 24. https://bodhi.fedoraproject.org/updates/FEDORA-2017-ffab28f335

Comment 18 Fedora Update System 2017-04-13 15:20:09 UTC
php-gettext-gettext-3.5.9-5.fc25 has been pushed to the Fedora 25 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-2017-534f555ca4

Comment 19 Fedora Update System 2017-04-13 15:20:45 UTC
php-gettext-gettext-3.5.9-5.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-2017-ffab28f335

Comment 20 Fedora Update System 2017-04-13 17:21:56 UTC
php-gettext-gettext-3.5.9-5.fc26 has been pushed to the Fedora 26 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-2017-1f428f5e35

Comment 21 Fedora Update System 2017-04-17 15:54:29 UTC
php-gettext-gettext-3.5.9-5.fc26 has been pushed to the Fedora 26 stable repository. If problems still persist, please make note of it in this bug report.

Comment 22 Fedora Update System 2017-04-22 08:19:35 UTC
php-gettext-gettext-3.5.9-5.fc24 has been pushed to the Fedora 24 stable repository. If problems still persist, please make note of it in this bug report.

Comment 23 Fedora Update System 2017-04-22 09:22:13 UTC
php-gettext-gettext-3.5.9-5.fc25 has been pushed to the Fedora 25 stable repository. If problems still persist, please make note of it in this bug report.


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