Support more versions of symfony/polyfill-mbstring

[rudolfbyker]

Would you consider supporting a wider range of versions? Dependency symfony/polyfill-mbstring is currently locked to v1.10.*. This changed between releases 2.1.6 and 2.1.7. I'm using this in a Drupal project. Drupal requires v.1.15.0. Therefore I am stuck with using 2.1.6.

$ composer why symfony/polyfill-mbstring
...
drupal/core-recommended    8.9.x-dev  requires  symfony/polyfill-mbstring (v1.15.0)  
seboettg/citeproc-php      v2.1.6     requires  symfony/polyfill-mbstring (^1.3)    
...

[rudolfbyker]

The reason for the fixed version required by Drupal is explained here: https://packagist.org/packages/drupal/core-recommended

[rudolfbyker]

I tried citeproc-php without the polyfill, since my server has the mbstring extension. Drupal works fine without it, but citeproc-php keeps using the Polyfill when the extension is available. So I think there are two things that need to change:

1. Support a wider range of versions for symfony/polyfill-mbstring. (This issue)
2. When the PHP Extention is available, don't even try to load the polyfill. This will allow you to move it from "require" to "suggest" in composer.json. https://getcomposer.org/doc/04-schema.md#suggest

Example:

{
    "suggest": {
        "symfony/polyfill-mbstring": "Allows citeproc-php to function without the PHP mbstring extension."
    }
}

I see you are using the polyfill class directly. Although that is not wrong, it does prevent the use of the PHP extension when available. The extension is probably faster then the native PHP implementation provided by the polyfill.

[rudolfbyker]

Hi again, I saw your commits, and tried it out, but I'm getting some errors.

Test php file:

<?php

include __DIR__ . "/../vendor/autoload.php";
use Seboettg\CiteProc\StyleSheet;
use Seboettg\CiteProc\CiteProc;

ini_set('display_errors', 1);
ini_set('display_startup_errors', 1);
error_reporting(E_ALL);

$data = json_decode('
[
    {
        "title": "Becoming partners",
        "title-short": "Becoming partners",
        "type": "book",
        "author": [
            {
                "family": "Rogers, Carl R."
            }
        ],
        "publisher": "Constable",
        "publisher-place": "London",
        "issued": {
            "date-parts": [
                [
                    "1973"
                ]
            ],
            "literal": "1973"
        },
        "id": "ITEM-1"
    }
]
');
$style = StyleSheet::loadStyleSheet('society-of-biblical-literature-fullnote-bibliography');
$citeProc = new CiteProc($style, "en-US");

?>

<h1>Lorem Ipsum</h1>
<?php
$citationItems = json_decode('
[
    {
        "id": "ITEM-1",
        "label": "page",
        "locator": "123"
    }
]
');
$cite1 = $citeProc->render($data, "citation", $citationItems);
?>
<p><?php echo $cite1 ?></p>

Test steps:

• git checkout #commit
• rm -rf vendor composer.lock
• composer install --no-suggest
• Serve a page with the abovementioned PHP code.

Results for various commits:

4e55151193335cffbcaa9c9804144f451e838bb7 Change symfony/polyfill-mbstring requirement from v1.10.* to ^1.10

No errors. We get symfony/polyfill-mbstring v1.17.0 now, which is good. Locators not working, but that's OK, since it's not merged yet at this commit.

489ce70ddf22f538b1d52f9698853218d7a8c877 implements the suggestions from issue #85

This commit moves the polyfill to "suggests" and tries to make the module work without the polyfill, but the polyfill gets installed anyway, because of this dependency tree:

$ composer why --tree symfony/polyfill-mbstring
symfony/polyfill-mbstring v1.17.0 Symfony polyfill for the Mbstring extension
└──symfony/console v4.2.12 (requires symfony/polyfill-mbstring ~1.0)
   └──satooshi/php-coveralls v1.1.0 (requires symfony/console ^2.1 || ^3.0 || ^4.0)
      └──seboettg/citeproc-php dev-489ce70ddf22f538b1d52f9698853218d7a8c877 (requires (for development) satooshi/php-coveralls ^1)

Also, this warning appears twice: Warning: mb_detect_encoding(): Illegal argument in /var/www/html/citeproc-php/src/Util/StringHelper.php on line 145. I tried to debug it, but could not find the cause. At least we know this commit is to blame.

Maybe moving the polyfill to "suggests" is a bad idea after all. The previous commit, 4e55151193335cffbcaa9c9804144f451e838bb7 works well enough to solve this issue.

[bryjbrown]

Hi, nothing constructive to add to this PR other than I ended here because I too am using Drupal and want to use citeproc-php for my project, but cannot install it through composer due to it requiring symfony/polyfill-mbstring v1.10. when Drupal is already using v1.18..

[rudolfbyker]

@bryjbrown feel free to use my fork for now: https://github.com/refstudycentre/citeproc-php/commit/4e55151193335cffbcaa9c9804144f451e838bb7

I made two suggestions, one of which fixed the Drupal compatibility (4e55151193335cffbcaa9c9804144f451e838bb7) and one of which just broke more things (489ce70ddf22f538b1d52f9698853218d7a8c877). @seboettg implemented both. Hopefully he will revert to 4e55151193335cffbcaa9c9804144f451e838bb7 at some point, which is definitely the more stable commit.

[seboettg]

Hi @rudolfbyker! I merged your PR. If you can confirm the fix, please close this ticket.

[rudolfbyker]

Yes, it works!

$ composer require seboettg/citeproc-php:dev-master
...
  - Installing seboettg/citeproc-php (dev-master 2a4dcae): Cloning 2a4dcaed5a from cache
...

$ composer why symfony/polyfill-mbstring
drupal/core-recommended    9.0.3       requires  symfony/polyfill-mbstring (v1.17.0)  
seboettg/citeproc-php      dev-master  requires  symfony/polyfill-mbstring (^1.10)    
symfony/console            v4.4.9      requires  symfony/polyfill-mbstring (~1.0)     
symfony/dom-crawler        v4.4.11     requires  symfony/polyfill-mbstring (~1.0)     
symfony/http-foundation    v4.4.9      requires  symfony/polyfill-mbstring (~1.1)     
symfony/mime               v5.1.0      requires  symfony/polyfill-mbstring (^1.0)     
symfony/polyfill-intl-idn  v1.17.0     requires  symfony/polyfill-mbstring (^1.3)     
symfony/translation        v4.4.9      requires  symfony/polyfill-mbstring (~1.0)     
symfony/validator          v4.4.9      requires  symfony/polyfill-mbstring (~1.0)     
symfony/var-dumper         v5.1.0      requires  symfony/polyfill-mbstring (~1.0)     
twig/twig                  v2.12.5     requires  symfony/polyfill-mbstring (^1.3)