Add ability to trim at sentence break when using words (D8)

Created on 10 October 2016, about 8 years ago
Updated 2 June 2023, over 1 year ago

There is currently patch for d7 https://www.drupal.org/node/2059085

Could we get similar functionality to D8? I think the functionality is there in the core or DS because when using default "Summary or trimmed" it worked (was default behaviour).

Feature request
Status

Needs review

Version

2.0

Component

Code

Created by

🇪🇪Estonia hkirsman

Live updates comments and jobs are added and updated live.
Sign in to follow issues

Merge Requests

Comments & Activities

Not all content is available!

It's likely this issue predates Contrib.social: some issue and comment data are missing.

  • 🇮🇪Ireland lostcarpark

    There seems to be a lot of overlap between this and Trim paragraph containing word count Closed: duplicate . It would be worth reviewing both as they both add a very similar option. This change seems to explicitly consider paragraphs in the splitting, which I don't think are part of the other.

    Regarding the paragraph split, it seems to only use the <p> tag. I think it would be valid to include a list of paragraph divider tags, as headings, lists or other common types are not currently considered.

    The change also seems to have sentence breaks hard coded. It would be good to use the sentence break characters constant defined 🐛 Error cropping Japanese Needs work , when that change gets merged.

  • First commit to issue fork.
  • @mjgruta opened merge request.
  • 🇵🇭Philippines mjgruta

    re-rolled patch #15 to 2.1.x-dev

  • @mjgruta opened merge request.
  • Status changed to Needs work over 1 year ago
  • 🇺🇸United States ultimike Florida, USA

    @mjgruta,

    We appreciate your efforts on this issue, but I'm setting it back to "Needs work" for a couple of reasons:

    1. There is no test coverage in the MR. All bug fixes and feature additions to the Smart Trim module require test coverage.
    2. I don't think we have consensus on whether or not "sentence" should be a new trim unit (similar to "characters" and "words") or the functionality should be added via a separate checkbox (as the current MR does).

    I'm not crazy about the logic of the current MR, as if the formatter is configured for a 50-character trim, but the sentence is 200 characters (not uncommon), then the trimmed text is going to be 4 times longer than the 50-character settings. I think this will lead to undesired results in a non-trivial number of situations. Therefore, I'd like to see this functionality implemented using "sentence" as a new trim unit. This would also have the added benefit of not adding a new checkbox to the formatter config UI.

    -mike

  • 🇩🇪Germany Anybody Porta Westfalica
  • 🇺🇸United States gmercer

    Re-rolled 2815981-15.patch for 2.1.0

  • 🇮🇪Ireland lostcarpark

    I can understand attaching a patch file might be useful for people to apply to live sites, but shouldn't the patch be taken from MR !60 rather than the previous #15 patch?

  • First commit to issue fork.
  • Pipeline finished with Failed
    10 months ago
    Total: 246s
    #108430
  • Status changed to Needs review 10 months ago
  • 🇵🇭Philippines mjgruta

    Updates from #30 break the patch. I have merged the latest code.

  • Pipeline finished with Failed
    10 months ago
    Total: 354s
    #110002
  • Status changed to Needs work 10 months ago
  • 🇵🇭Philippines mjgruta

    I just noticed the comment from mike. Reverting the status to Needs work.

  • Pipeline finished with Failed
    10 months ago
    Total: 320s
    #110135
  • Pipeline finished with Failed
    10 months ago
    Total: 303s
    #110703
  • 🇮🇪Ireland lostcarpark

    I've made a few suggestions.

    I think we also need some test cases that explicitly verify this.

  • 🇵🇭Philippines mjgruta

    Created a diff from commit 0779410f while fixing MR #60 to prevent the composer from failing to apply the patch.

  • Pipeline finished with Failed
    10 months ago
    Total: 261s
    #111240
  • 🇵🇭Philippines mjgruta

    Created another patch file from commit 975c9a47. This should fix the comments from James.

  • Pipeline finished with Failed
    10 months ago
    Total: 322s
    #111645
  • 🇵🇭Philippines mjgruta

    fixes $text_format type from null to string.

  • Pipeline finished with Failed
    10 months ago
    Total: 388s
    #114164
  • 🇺🇸United States joshf

    This patch fixes the fatals caused by the previous change.

  • Pipeline finished with Failed
    10 months ago
    Total: 232s
    #114258
  • 🇵🇭Philippines mjgruta

    @josh Sorry, the fix needs to be done on the field formatted viewElements as it is trying to pass the $item->format directly which will return null if the item doesn't have a format.
    I have updated the code.

  • Pipeline finished with Failed
    10 months ago
    Total: 311s
    #114264
  • Pipeline finished with Failed
    10 months ago
    #114267
  • 🇵🇭Philippines mjgruta

    Fixed error in phpstan validation.

  • 🇺🇸United States joshf

    Hi @mjgruta, thanks; your latest change fixes the fatals I was getting with the previous one.

  • Pipeline finished with Failed
    9 months ago
    Total: 228s
    #116745
  • 🇺🇸United States joshf

    This fixes an error that popped up in our automated testing.

  • Pipeline finished with Failed
    9 months ago
    Total: 254s
    #116747
  • 🇷🇺Russia Andrew Answer Novosibirsk

    +1 to release this feature in the new module version.

  • 🇧🇪Belgium waropd

    Created a patch from #41 to use

  • 🇧🇪Belgium Tim Lammar

    Rework of #43 to work with version 2.2.0

Production build 0.71.5 2024