Problem/Motivation

The constant PHP_TYPOGRIFY_VERSION is defined in typogrify.module, but used only in src/Plugin/Filter/TypogrifyFilter.php. We may as well define it there.

Proposed resolution

  1. Remove the define() line from the .module file.
  2. Define a (public) constant in the filter plugin.
  3. Update the usage in the filter plugin.

Pay attention to coding standards: see the testbot results from #3124619-2: Add typogrify.css when the Typogrify filter is used. In order to avoid merge conflicts/rerolls, this issue is postponed on that one.

Remaining tasks

User interface changes

None

API changes

  • Remove the (global) module constant PHP_TYPOGRIFY_VERSION.
  • Add the class constant Drupal\typogrify\Plugin\Filter\TypogrifyFilter::TYPOGRIFY_VERSION

Data model changes

None

CommentFileSizeAuthor
#7 3124621-7.patch1.15 KBbenjifisher
#7 diff-3124621-5-7.txt599 bytesbenjifisher
#5 3124621-5.patch1.14 KBbenjifisher
#5 interdiff-3124621-3-5.txt327 bytesbenjifisher
#3 3124621-3.patch1.12 KBbenjifisher
#3 typogrify-config-form.png13.08 KBbenjifisher

Comments

benjifisher created an issue. See original summary.

benjifisher’s picture

benjifisher
he/him or they/them
Primary language English
Location Boston area
commented
Assigned: Unassigned » benjifisher
Status: Postponed » Active

The current patch on #3124619: Add typogrify.css when the Typogrify filter is used does not touch typogrify.module, so there is no problem with merge conflicts. I am un-postponing this issue, and I will work on it now.

benjifisher’s picture

benjifisher
he/him or they/them
Primary language English
Location Boston area
commented
Assigned: benjifisher » Unassigned
Status: Active » Needs review
StatusFileSize
new typogrify-config-form.png13.08 KB
new 3124621-3.patch1.12 KB

The attached patch implements the Proposed resolution in the issue summary. It completely removes typogrify.module, so we do not have to worry about coding standards in that file.

To test, edit a format (e.g., /admin/config/content/formats/manage/basic_html) and enable the Typogrify filter. At the end of the config form, you should see the value of the new constant ('1.0') as in this screenshot:

Screenshot showing the bottom of the Typogrify config form

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Status: Needs review » Reviewed & tested by the community

It completely removes typogrify.module

🥳

+++ b/src/Plugin/Filter/TypogrifyFilter.php
@@ -41,6 +41,11 @@ use Drupal\Core\Url;
+  /**
+   * The version of the Typogrify library being used.
+   */
+  const TYPOGRIFY_VERSION = '1.0';

An übernit here would be that it should in principle contain

  * @var string
benjifisher’s picture

benjifisher
he/him or they/them
Primary language English
Location Boston area
commented
Status: Reviewed & tested by the community » Needs review
StatusFileSize
new interdiff-3124621-3-5.txt327 bytes
new 3124621-5.patch1.14 KB

Thanks for the review. We may as well get it right.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Status: Needs review » Reviewed & tested by the community

Hehe :)

benjifisher’s picture

benjifisher
he/him or they/them
Primary language English
Location Boston area
commented
StatusFileSize
new diff-3124621-5-7.txt599 bytes
new 3124621-7.patch1.15 KB

@Wim Leers:

Thanks for reviewing. Sometimes the small changes are dangerous because we get careless.

This patch needs a reroll after #3124616: Fix coding standards in Typogrify. Given the size of the two patches, I am happy to be doing the reroll on this issue. That is, I am glad I decided to fix that issue first.

I am attaching a diff (not an interdiff). The only differences between this patch and the one in #5 is that context lines now use the short array syntax.

I am leaving the status at RTBC. If the testbot does not object, then I will commit this patch.

  • benjifisher committed 7999e00 on 8.x-1.x 
    Issue #3124621 by benjifisher, Wim Leers: Change PHP_TYPOGRIFY_VERSION...
benjifisher’s picture

benjifisher
he/him or they/them
Primary language English
Location Boston area
commented
Status: Reviewed & tested by the community » Fixed

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.