seboettg / citeproc-php

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

Date parsing issue patch #56

Closed silence-cube closed 5 years ago

silence-cube commented 6 years ago

Bug reports:

We had issues that a date without month and day always resulted in 1. Jan for publications, even though the date parser would be able to output a date without day/month (just year). Then we found out, that this was caused by the fact that dates are ALWAYS parsed by PHPs built in date parser and is stored as a date object with day/month. After removing this part from the code, the dates were generated properly as they should (only a year if there was no month/day in the data).

Solution

It seems to be a bug to us, that DateTime MUST be a full date. The PHP DateTime object is anyway not used anywhere, so we could simply remove the inheritance without any downsides.

--- a/src/Seboettg/CiteProc/Rendering/Date/DateTime.php 2018-04-18 08:20:13.000000000 +0200
+++ b/src/Seboettg/CiteProc/Rendering/Date/DateTime.php 2018-07-06 10:46:24.838880400 +0200
@@ -10,9 +10,7 @@
 namespace Seboettg\CiteProc\Rendering\Date;

-use DateTimeZone;
-
-class DateTime extends \DateTime
+class DateTime
 {
     /**
      * @var int
@@ -37,10 +35,9 @@
      */
     public function __construct($year, $month, $day)
     {
-        parent::__construct("$year-$month-$day", new DateTimeZone("Europe/Berlin"));
-        $this->year = intval(self::format("Y"));
-        $this->month = intval(self::format("n"));
-        $this->day = intval(self::format("j"));
+        $this->year = intval($year);
+        $this->month = intval($month);
+        $this->day = intval($day);
     }

     /**
@@ -84,7 +81,6 @@
         $this->year = $year;
         $this->month = $month;
         $this->day = $day;
-        parent::setDate($year, $month, $day);
         return $this;
     }
seboettg commented 6 years ago

I guess that isn't an issue. A date is a date, containing year, month and a day. What you want to have is just a year. You may use a stylesheet which just renders years instead of whole dates.

silence-cube commented 6 years ago

Some CSL styles render the day and month if they are provided in the data, if not they render just the year. Without this patch, citeproc-php will ALWAYS create a day and month (=1. Jan) and hand it over to the CSL style, even if it does not exist in the input data.

It seems to me that the DateTime parsing was added later on, causing this issue. When I look at the code, it seems that it was originally intended having no month or day:

    /**
     * @var int
     */
    private $year = 0;

    /**
     * @var int
     */
    private $month = 0;

    /**
     * @var int
     */
    private $day = 0;

The day wouldn't be rendered if the constructor wouldn't force the date parsing via PHP DateTime object (with a hardcoded timezone "Europe/Berlin").

    protected function renderDay(DateTime $date)
    {
        if ($date->getDay() < 1 || $date->getDay() > 31) {
            return "";
        }

This piece of code will never return an empty string, because the day will be always in the accepted range (because of DateTime object parsing).

seboettg commented 6 years ago

Thank you for your quick answer. I'm skeptical because all tests are running. I can't believe that not a single test would detect this problem, if it is a problem. I would like to get deeper into the matter and will check the CSL specification. Additionally I would like to compare citeproc-php with the behaviour of citeproc-js. For this I need input data and a stylesheet.

seboettg commented 5 years ago

@silence-cube: please post example meta data and the name of the used stylesheet.

silence-cube commented 5 years ago

@seboettg We're unable to reproduce it with the latest version, I will check older versions again. But anyway you can ignore this issue for the next release then.

seboettg commented 5 years ago

@silence-cube I'm going to close this ticket now. If you can reproduce this issue in future, feel free to reopen the ticket. Thanks :)