social_swiftmail and PHP 8.1, wrong parameter for border radius

Created on 17 February 2023, almost 2 years ago
Updated 21 August 2023, over 1 year ago

Problem/Motivation

1. social_swiftmail produces some warnings when using PHP 8.1 based on "Passing null to non-nullable internal function parameters is deprecated"
2. function social_swiftmail_preprocess_swiftmailer tries to get a border radius based on:
$border_radius = Xss::filter(theme_get_setting('border_radius', $theme_id));
As far as I see nad know a setting border_radius does not exits in socialbase or socailblue and should be replaced by "card_radius"

Steps to reproduce

Setup Open Social with PHP 8.1, make sure social_swiftmail is enabled and send emails using SwiftMail.

Proposed resolution

Check that value is not null before continue processing

πŸ› Bug report
Status

Fixed

Version

11.9

Component

Code (back-end)

Created by

πŸ‡©πŸ‡ͺGermany slowflyer

Live updates comments and jobs are added and updated live.
  • PHP 8.1

    The issue particularly affects sites running on PHP version 8.1.0 or later.

Sign in to follow issues

Comments & Activities

  • Issue created by @slowflyer
  • πŸ‡¨πŸ‡¦Canada Liam Morland Ontario, CA πŸ‡¨πŸ‡¦
  • πŸ‡©πŸ‡ͺGermany donquixote

    I don't like `!= null`. It obfuscates the type conversions. See https://3v4l.org/fL8Dp
    E.g. `'' == null`, but `'' !== null`.

    Either we only want to detect NULL, then we use `=== NULL` or `!== NULL`.
    Or we want to detect false-ish values, then we could use `if ($v)` or `if (!$v)` or `if ((bool) $v)`.
    (We could also use `empty()` but this hides undefined variables.)

    Also NULL would be uppercase with Drupal coding standards. And there is a missing space after first `if`.

    As far as I see nad know a setting border_radius does not exits in socialbase or socailblue and should be replaced by "card_radius"

    Then other instances of 'border_radius' also need to be replaced.
    E.g. here in the same function social_swiftmail_preprocess_swiftmailer():

      $variables['border_radius'] = $border_radius;
    

    As far as I can see, setting this in variables will have no effect. But then again, I don't see either border_radius nor card_radius in the swiftmailer template. Maybe I am missing something?

    ----

    I see one place where 'card_radius' is used when generating in-page CSS, in `improved_theme_settings_page_attachments()`.
    But this has nothing to do with swiftmailer.

  • πŸ‡©πŸ‡ͺGermany donquixote

    Btw a shortcut would be this:

      $border_radius = Xss::filter(theme_get_setting('border_radius', $theme_id) ?? '0');
    

    It would mean that the Xss::filter() is still executed even for zero, but I think it is acceptable.
    I am putting here '0' (string) and not 0 (integer) because the filter function will convert it to string anyway.

    But perhaps all of this is just a leftover that needs to be cleaned up?

  • πŸ‡©πŸ‡ͺGermany donquixote

    For the `$disabled_greeting_keys`:

    • The stored setting could be NULL or '' (empty string). Both should have the same effect.
    • The stored setting could have trailing line breaks or even trailing spaces in line endings. In that case, or in case the setting itself is an empty string (as above), the exploded array would contain an empty string item.
      I assume in either case we want to ignore these empty string parts.
    • The `$message['key']` might also be an empty string.
      In that case, I assume we want to treat it the same as if it is NULL, so that the heading is always shown.

    I say all of this from looking at the code, I did not actually test anything.

    Btw the control flow in this part of the function is a bit cluttered and redundant, and the different parts are not in an ideal order.
    It could be refactored like this:

    
      // Try to add a heading message.
      $display_name = $user?->getDisplayName()
        ?? $message['params']['context']['display_name']
        ?? NULL;
    
      if (!$display_name) {
        return;
      }
    
      if (!empty($message['key'])) {
        $disabled_greeting_keys = $social_swiftmail_config->get("disabled_user_greeting_keys") ?? '';
        $disabled_greeting_keys = explode("\r\n", $disabled_greeting_keys);
        if (in_array($message['key'], $disabled_greeting_keys)) {
          return;
        }
      }
    
      $variables['heading'] = t(
        'Hi <strong>@display_name</strong>',
        ['@display_name' => $display_name],
        ['langcode' => $message['langcode']],
      );
    

    Note that:

    • I appended `?? ''` to to $social_swiftmail_config->get("disabled_user_greeting_keys") ?? ''.
    • I replaced the isset($message['key']) with !empty($message['key']) to also cover empty string. But we have to look the possible empty-ish values this might have, and what the consequences of that would be.
  • πŸ‡ΊπŸ‡¦Ukraine kashandarash

    hello, there is PR for this with two border-radius https://github.com/goalgorilla/open_social/pull/3482/files

  • Status changed to Needs review over 1 year ago
  • Status changed to Needs work over 1 year ago
  • πŸ‡ΊπŸ‡ΈUnited States SocialNicheGuru

    Uploaded a patch based on PR that applies to 11.9.x

    • ribel β†’ committed 71dad44a on 11.10.x
      Issue #3342642: update variables for email tempalte to pass border-...
  • Status changed to Fixed over 1 year ago
  • πŸ‡³πŸ‡±Netherlands tbsiqueira Rotterdam
  • πŸ‡ΊπŸ‡¦Ukraine ribel πŸ‡ΊπŸ‡¦Lviv
  • Automatically closed - issue fixed for 2 weeks with no activity.

Production build 0.71.5 2024