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

Issue fork social-3342642

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

Comments

slowflyer created an issue. See original summary.

slowflyer’s picture

StatusFileSize
new1.72 KB
liam morland’s picture

Issue tags: -social_swiftmail php81 +PHP 8.1
donquixote’s picture

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.

donquixote’s picture

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?

donquixote’s picture

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.
  • I inlined the $options and $replace. We don't actually need to do this.
    The main point was to move this stuff down because it can be skipped if any of the conditions above fail.
kashandarash’s picture

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

kashandarash’s picture

Status: Active » Needs review
socialnicheguru’s picture

Version: 11.8.x-dev » 11.9.x-dev
Status: Needs review » Needs work
StatusFileSize
new1.32 KB

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-...

tbsiqueira credited ribel.

tbsiqueira’s picture

Status: Needs work » Fixed

Status: Fixed » Closed (fixed)

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