seboettg / citeproc-php

Full-featured CSL 1.0.1 processor for PHP
MIT License
75 stars 39 forks source link

Printing roman numeric when input is also roman #71

Closed ghost closed 4 years ago

ghost commented 4 years ago

Bug reports:

The isNumeric constraint does a good job identifying that the "II" in "volume":"II" is a number, however, when printing it the code does not recognize the "II" as a number in (Number.php), and the "II" is not printed at all.

case 'roman':
    $var = $data->{$this->variable};
    if (preg_match("/\s*(\d+)\s*([\-\-\&,])\s*(\d+)\s*/", $var, $matches)) {
        $num1 = Util\NumberHelper::dec2roman($matches[1]);
        $num2 = Util\NumberHelper::dec2roman($matches[3]);
        $text = $this->buildNumberRangeString($num1, $num2, $matches[2]);
    } else {
        $text = Util\NumberHelper::dec2roman($var);
    }
    break;

Note that the $this->variable has the value "II" at this point.

Maybe the solution is to simply convert the "II" into a number using the same logic in the isNumeric constraint, and then applying the correct format... ?

...and I do appreciate that we could simply have "volume":"2" instead of "volume":"II"...but I think both should work.

What do you think?

Used CSL stylesheet:

modern-humanities-research-association.csl specifically this part

<choose>
  <if is-numeric="volume">
    <number variable="volume" form="roman" font-variant="small-caps"/>
  </if>
  <else>
    <text variable="volume" font-variant="small-caps"/>
  </else>
</choose>

Used CSL metadata

[{
   "id":"7021352\/UTSP92QZ",
   "type":"book",
   "title":"Early Sixteenth-Century Sacred Music from the Papal Chapel",
   "collection-title":"Corpus Mensurabilis Musicae",
   "collection-number":"95-2",
   "publisher":"American Institute of Musicology, Ha\u0308nssler-Verlag",
   "publisher-place":"Neuhausen-Stuttgart",
   "volume":"II",
   "event-place":"Neuhausen-Stuttgart",
   "language":"en",
   "editor":[
      {
         "family":"Josephson",
         "given":"Nors S."
      }
   ],
   "issued":{
      "date-parts":[
         [
            1982
         ]
      ]
   }
}]
ghost commented 4 years ago

Hi @seboettg,

tested with master branch and this fix/improvement is working as expected, at least for the scenario of this issue :)

Thanks