seboettg / citeproc-php

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

Date.php needs to check that $raw is set (maybe everywhere?) #68

Closed bseeger closed 4 years ago

bseeger commented 4 years ago

I'm using this library via another library inside of Drupal 7.

Bug reports:

I think that this line needs to check $raw before it tries to use it. If the $raw is not set here, I see this error bubble up:

(these say line 126 & 127 - that's because I'm using version 2.1.7 - I found the relevant lines on master).

Notice: Undefined property: stdClass::$raw in Seboettg\CiteProc\Rendering\Date\Date->render() (line 126 of /var/www/html/sites/all/modules/islandora/islandora_scholar/modules/citeproc/vendor/seboettg/citeproc-php/src/Seboettg/CiteProc/Rendering/Date/Date.php).

Notice: Undefined property: stdClass::$raw in Seboettg\CiteProc\Rendering\Date\Date->render() (line 127 of /var/www/html/sites/all/modules/islandora/islandora_scholar/modules/citeproc/vendor/seboettg/citeproc-php/src/Seboettg/CiteProc/Rendering/Date/Date.php).

I think it needs to check $raw, like this line does: https://github.com/seboettg/citeproc-php/blob/master/src/Rendering/Date/Date.php#L187

bseeger commented 4 years ago

However, maybe it's not okay for $raw to be empty at this point?

seboettg commented 4 years ago

Hi @bseeger! Thank you for that bug report. In order to be able to reproduce this issue, I need the used input data and the used Stylesheet. It is important to generate test cases that can reproduce the erroneous behavior. That way I can minimize side effects and possibly expose more issues that might happen after applying a bug fix.

bseeger commented 4 years ago

Hi @seboettg - I'll work on getting this information. I'm using citeproc-php in Islandora 7 and I think I can get the CSL for it, but I'm not sure yet how it's handing citeproc the data. I'll see what I can dig up and send to you.

seboettg commented 4 years ago

@bseeger, I just need the name of the used stylesheet. You do not need to paste the xml content here. I'm not sure how Islandora works, but citeproc-php consumes JSON input data based on the following schema: https://github.com/citation-style-language/schema/blob/master/csl-data.json. I need the data that lead to the mentioned error. Thanks!

bseeger commented 4 years ago

We're using the APA CSL and if I don't set date issued I see the above errors. (I'm going to enforce setting date issued, but still figured I'd tell you about this. :)

In terms of data - there's some code that translates MODS XML to JSON on the fly, so I don't have a data sample off hand.

Code that does the translation: https://github.com/Islandora/islandora_scholar/blob/7.x/modules/citeproc/includes/converter.inc

bseeger commented 4 years ago

I was able to look at a PHP object that's turned into JSON to be sent:

object(stdClass)#186 (3) {
  ["title"]=>
  string(10) "TEST: test"
  ["type"]=>
  string(15) "article-journal"
  ["issued"]=>
  object(stdClass)#192 (0) {
  }
}

In the event that our users don't set Date Issued, I can see we are sending in an "issued" key with an empty object, which is probably triggering the warnings. I think we are trying to set a default of '' so that it shows up in our data. So this is kind of a boundary case, really, and $raw is probably set most time. Hmmm...

For example, the code that creates the above PHP object is trying to do this:

object(stdClass)#186 (3) {
  ["title"]=>
  string(10) "TEST: test"
  ["type"]=>
  string(15) "article-journal"
  ["issued"]=>
  object(stdClass)#197 (1) {
    ["string-literal"]=>
    string(0) ""
  }
}

But since it's a 0 length string, I think PHP just removes it. And then it gets passed to your code.

So, this issue is not in your code specifically, though those warnings do happen if $raw is not set.

seboettg commented 4 years ago

@bseeger I was able to reproduce the described behavior. I built the following fix: https://github.com/seboettg/citeproc-php/blob/master/src/Rendering/Date/Date.php#L130 (from line 130 to 138). I'm going to prepare the release now. As soon as the release is done, you need to run "composer update" in your islandora appliaction to update citeproc-php.

If you have further question or the issue still appears, feel free to reopen this ticket, or open a new one.

P.S.: Don't forget to star this repository on Github. On this way, you can help to make citeproc-php more popular and more spread.