Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ISSUE-87: CSL-citations as a twig extension & a field formatter: a tale of two converging paths #192

Merged
merged 49 commits into from
Jun 27, 2022

Conversation

aksm
Copy link
Contributor

@aksm aksm commented Apr 26, 2022

Beginning work on #87. Still a long way to go. Currently some of the styles (list of examples below) cause the first letter of each word in a title to be stripped:

chicago-author-date
chicago-author-date-16th-edition
university-of-york-mla

It's possible that the following log notice is involved:

Notice: iconv(): Wrong charset, conversion from `ISO-8859-1' to `UTF-8//IGNORE' is not allowed in Symfony\Polyfill\Mbstring\Mbstring::mb_convert_case() (line 285 of /var/www/html/vendor/symfony/polyfill-mbstring/Mbstring.php).

@aksm aksm added enhancement New feature or request Twig Twig template processing metadata Meta(l) data IR Institutional repositories, what every scholar loves and fears labels Apr 26, 2022
@aksm aksm requested a review from DiegoPino April 26, 2022 20:17
@aksm aksm self-assigned this Apr 26, 2022
@aksm
Copy link
Contributor Author

aksm commented Apr 27, 2022

@DiegoPino I think I located bug involving words in titles being stripped of their first letters.

The following ternary evaluates to true, but then the true condition doesn't return the concatenated string (I'm guessing because of that Mbstring notice that occurs in the logs (mentioned above):

return in_array($encoding, self::ISO_ENCODINGS) ?
    Mbstring::mb_strtoupper($firstChar, $encoding).$then : $firstChar.$then;

The above is from here:

https:/seboettg/citeproc-php/blob/afdfbfd616b2ed3dd0f0e45feda72d0b007e9c8e/src/Util/StringHelper.php#L142-L144

Still looking into it. Will follow up as soon as I figure out more.

@DiegoPino
Copy link
Member

@aksm the problem is this here
https://stackoverflow.com/questions/63746822/why-does-iconv-returns-empty-string-in-php7-4-fpm-alpine-docker

@aksm
Copy link
Contributor Author

aksm commented Apr 27, 2022

@DiegoPino Hmm, I saw that and skipped, hoping it wasn't a devops/dependency issue. Do we go with the accepted answer?

@DiegoPino
Copy link
Member

Probably good to test with the accepted answer. Try running the command on your own local php container (without rebuilding, just the command live) and reload the container. See if that changes things. IF so, then I will rebuild both 7.4 and 8.0 to include an updated iconv library

@DiegoPino DiegoPino changed the base branch from 1.0.0-RC3 to 1.0.0 April 27, 2022 18:06
// $locale_options[$key] = $this->t($locale_string);
//}
// There's a better way to get this directory
$citation_style_directory = '/var/www/html/vendor/citation-style-language/styles-distribution';
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

citation-style-language library needs to be a composer dependency and yes, that is not a good way of getting that folder

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will fix my own comment here. If we go composer we will need a custom script to git clone that repository. Or we can ship this as a DevOps operation and have a configuration (global one for archipelago) to state where this is downloaded/present

Copy link
Member

@DiegoPino DiegoPino Apr 27, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, this is already solved in the libraries example

{
    "name": "vendor-name/program-name",
    "repositories": [
        {
            "type": "package",
            "package": {
                "name": "citation-style-language/locales",
                "version":"1.0.0",
                "source": {
                    "type": "git",
                    "url": "https:/citation-style-language/locales.git",
                    "reference": "master"
                }
            }
        },
        {
            "type": "package",
            "package": {
                "name": "citation-style-language/styles-distribution",
                "version":"1.0.0",
                "source": {
                    "type": "git",
                    "url": "https:/citation-style-language/styles-distribution.git",
                    "reference": "master"
                }
            }
        }
    ],
    "require": {
        "citation-style-language/locales":"@dev",
        "citation-style-language/styles-distribution":"@dev",
        "seboettg/citeproc-php": "^2"
    }
}

so this needs to go in our composer, not just the "seboettg/citeproc-php": "master"

DiegoPino and others added 7 commits April 28, 2022 09:18
…t using IIIF Presentation Manifest API 3.0

everything seems to work except the aspect ratio (height) is wrong.
…IIF Presentation API 3 Creative Works Series Manifest' as the metadata display template
…rifying that the getImagesListApi3 works with the 'IIIF Presentation API 3 Creative Works Series Manifest'
…irst structure range, if present, to define image order.

I noticed that the initial structures page ordering failed for PDF books because the IIIF 3.0 Presentation Manifest has a bug in assigning the page IDs to the structure range item ids. I added trapping for that, and
do not use the structure order if the ids don't match.
@DiegoPino
Copy link
Member

@DiegoPino Do I have this right for generating a unique ID for the element, or am I missing something?

You mean referencing an existing ID as here var theid = '#' + $(this).attr("id"); ?

@aksm
Copy link
Contributor Author

aksm commented May 6, 2022

@DiegoPino Do I have this right for generating a unique ID for the element, or am I missing something?

You mean referencing an existing ID as here var theid = '#' + $(this).attr("id"); ?

More on the other side, i.e. generating a unique ID that won't conflict if there are multiple blocks:

      $uniqueid =
        'bibliography-' . $items->getName(
        ) . '-' . $nodeuuid . '-' . $delta;

@@ -10,6 +10,7 @@
use Drupal\Core\Session\AccountInterface;
use Drupal\Core\Template\TwigEnvironment;
use Drupal\format_strawberryfield\EmbargoResolverInterface;
use Drupal\pathauto\MessengerInterface;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi, this interface is not the right one for the Messenger. Check other plugins or inspect $this->messenger()

// Get the list of style files.
$style_list = $this->fileSystem->scanDirectory($citation_style_directory, '/\.(csl)$/i', ['recurse' => FALSE, 'key' => 'name']);
# Generate a list of select options and push in the styles.
$csl_exists = is_dir($csl_root);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good

$style_options[$style_name] = $this->t($style_title);
if (!$csl_exists) {
$this->messenger()->addWarning('Please run "drush archipelago-download-citeproc-dependencies" before using this formatter.', 'warning');
} else {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

DCS (Drupal coding standards), the else needs to go in a next line

$style_options[$style_name] = $this->t($style_title);
}
// Alphabetize them.
asort($style_options);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good!

'#default_value' => $this->getSetting('citationstyle'),
],
'localekey' => [
'#type' => 'textfield',
'#title' => $this->t('Provide a metadata key to use as the locale (language) for citations.'),
'#default_value' => $this->getSetting('localekey'),
'#required' => FALSE,
'#disabled' => !$csl_exists,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very good

$render_bibliography['bibliography'] = [
'#markup' => \Drupal\Core\Render\Markup::create($bibliography),
];
$rendered_bibliography = \Drupal::service('renderer')->render($render_bibliography);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@aksm this is great!!. Hint. Try returning it without the -> render and check it out. Twig should be able to render Render arrays directly. If that does not work out let me know!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@DiegoPino Tried that at one point and got the Return value must be of type string error. It looks like that only works with functions, not filters? Am I missing something?

@aksm
Copy link
Contributor Author

aksm commented May 26, 2022

@DiegoPino I feel this is complete for the most part so will start on the next issue and come back with fresh eyes to review for mistakes/additions. Thanks!

@aksm
Copy link
Contributor Author

aksm commented Jun 1, 2022

@DiegoPino pinging you for review when you're ready.

@DiegoPino DiegoPino merged commit 755a56d into esmero:1.0.0 Jun 27, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request IR Institutional repositories, what every scholar loves and fears metadata Meta(l) data Twig Twig template processing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants