Contrib.social

Docs and Code improvements, dead code removal, some fixes.

Open on Drupal.org →
Created on 29 November 2019, almost 5 years ago
Updated 18 February 2023, over 1 year ago

While debugging some mailsystem / mimemail / smtp issues, I did the following updates to mimemail:

In general:

  1. move assignments out of condition statements -- it makes debugging harder and is less clear
  2. Use null coalesce (??) and (?:) operators where possible to clarify code, now D8 requires Php7

MimeMail.php:

  1. Remove commented hook_mailengine implementation - no such hook exists and there seems no point in retaining it inline.
  2. Be more specific in looking for content-type: text/plain
  3. Delete commented-out engine code
  4. Delete line "removing ... by _drupal_wrap_mail_line()" as that function no longer exists and there is no newline to delete in reality
  5. Delete commented-out "simple_address" variant of $message[to] assignment.

MimeMailFormatHelper.php

  1. Improve function docs, especially types.
  2. Added tif, tiff extensions for the 'is image' test in mimeMailFile() and mimeMailUrl()
  3. Remove double quotes around email names -- because they aren't needed and for myself I was seeing doubled quoting.
  4. Remove "else{}" around code where the if {} has return'ed - clarity, performance.
  5. Reformat arrays to short form and on multiple lines -- clarity
  6. Replace string reps of namespaced classnames with ::class version of name.
  7. Reorder multipart headers so headers are always in the same order -- clarity.
  8. In some cases assign $content_type as the type and then use it, to keep within 80 col line limits and for clarity.
  9. In some cases, use === rather than == when this is safe.
  10. In function mimeMailFile and mimeMailUrl, pull all service and setting object requests to top of function for clarity and performance.
  11. Remove reference to RFC822 - that is the old number. Updated to 2822 to match other places in code, but current standard is RFC5322
  12. In function mimeMailFile invert no_access and not_in_public_path variables to avoid unclear double-negatives.
  13. In function mimeMailRfcHeaders, note that headers are required by the RFCs to end in CRLF - that sequence is not optional and should not be subject to a 'setting'. However inserting CR or CRLF as line ending here results in them being replaced by HTML entity in the DOM code
  14. In function mimeMailRfcHeaders, rework the wrapping code so it takes notice of the length of the key and avoids "magic numbers" like "20" and "60"; introduce named constant for max line length.
  15. In Return-path preg_match regex bump the top level domain length max to 8 chars from 4, and remove some unneeded backslashes.

mimemail-messages.html.twig

  1. Rework the code looking ar module length and key length to use more twig-centric constructs. However, it would be better to move setting html attributes out of twig entirely and use the Attributes class.

Notable @todos:

  1. "@todo: remove references to HTTP_HOST" when calculating mail ID. I am not sure what the options are but use of http_host does not feel right, especially if this is code that could conceivably be invoked by drush, etc.
  2. "@todo Fix PHPEA warning: 's' modifier is ambiguous, no '.' in pattern." in mimeMailExtractFiles()
  3. "@todo Searching the path is not what we want" while looking for a mail.css file to include
  4. "@todo This block is wrong for both D7 and D8 - 'engine' ..." while setting up the admin form.
  5. "@todo Port the mimemail_get_engines() function to D8." while setting up the admin form.
  6. In mimeMailHtmlBody() the parameter $subject is not used: is this a bug?
  7. Update README.txt to adapt instructions that are D7 specific.
📌 Task
Status

Fixed

Version

1.0

Component

Code

Created by

🇬🇧United Kingdom rivimey

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

Comments & Activities

Not all content is available!

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

  • Comment over 1 year ago →
    🇺🇸United States tr Cascadia

    Here is a list of patches from the zip file in #8, along with their dispositions:

    • 0001-Small-improvements-to-function-comments.patch
      This contains changes to documentation comments in src/Plugin/Mail/MimeMail.php and
      src/Utility/MimeMailFormatHelper.php. All those comments have been drastically improved since this issue, and this patch is no longer relevant.
    • 0002-Remove-unused-implementation-of-mimemail_engine.patch
      This was addressed by #3111496: Remove Mime Mail engine code →
    • 0003-Remove-references-to-unused-mimemail_engine-and-comm.patch
      The engine stuff was addressed by #3111496: Remove Mime Mail engine code → . The removal of hook_mail_post_process() has not been discussed, so I opened a new issue for it. See 📌 Remove hook_mail_post_process() Fixed .
    • 0004-Simplify-code-in-mimeMailAddress-remove-extraneous-a.patch
      Obsolete. This code has been replaced for porting to Drupal 10.
    • 0005-Use-where-possible.patch
      Opened 📌 Use === where possible Needs review .
    • 0006-code-style-reformat-arrays-to-multiline.patch
      This contains coding style changes to some arrays. That code has been fixed to conform with Drupal coding standards, and does not need further changes.
    • 0007-RFCs-state-line-ending-always-crlf-not-optional-so-b.patch
      This was fixed in #3153578: Use CRLF as line terminator in message body →
    • 0008-Modify-header-word-wrap-to-take-account-of-length-of.patch
      This was fixed in #3145542: MimeMailFormatHelper::mimeMailRfcHeaders() doesn't follow the RFC specification →
    • 0009-Use-of-null-coalesce-operator.patch
      Null coalesce was fixed by 📌 Coding standards: Use null coalesce operator instead of ternary operator Fixed , but this patch also changes many lines to use the short form of the ternary operator. I opened a new issue for that: 📌 Use short form of the ternary operator when possible Fixed .
    • 0010-Pull-drupal-services-to-top-of-functions-to-clarify-.patch
      The changes in this patch are similar to ones that have already been done by various issues when cleaning up the code and porting it to D10. There is one change to the regular expression for image files that seems out of scope, but is covered by the existing issue ✨ Improve image detection Needs work
    • 0011-Modify-condition-to-use-positive-logic-for-clarity.patch
      This has not been done yet, and I think your suggestions are a good improvement. I opened 📌 Modify conditions to use positive logic Fixed for this.
    • 0012-Simplify-regex-allow-longer-TLD-domain-names-note-re.patch
      This line of code will go away when ✨ Replace regular expression with mimeMailAddress() Fixed is committed. I encourage you to participate in that issue to ensure that issue solves any problem you might have here.
    • 0013-Clarify-setting-of-class-in-twig-html-template.patch
      I opened 📌 [meta] Improve Twig templates Active for this.
    • 0014-Pull-variable-initialisation-out-of-if-statement-for.patch
      I'm not going to change this right now. Your patch also changes the regexp and I don't want to get into that.
    • 0015-Dead-code-removal.patch
      Already removed.
    • 0016-Use-null-coalesce-operator.patch
      Null coalesce was fixed by 📌 Coding standards: Use null coalesce operator instead of ternary operator Fixed .
    • 0017-Pull-Drupal-service-initialisation-to-top-of-functio.patch
      This was fixed in a better way by #3136090: Use dependency injection in MimeMail Mail plugin →
    • 0018-Check-subject-line-doesn-t-contain-HTML.patch
      I don't know what this is, because as far as I can tell this line has been in this module since at least 2018. Subject lines are already stripped of HTML.
    • 0019-Changes-to-mimeHTMLBody-Possibly-broken.patch
      These are formatting changes which I don't feel are needed.



    It's really an awful lot of work to handle issues where there are 19 unrelated patches. In the future, I would appreciate 1 issue per problem, so each can be given due consideration and adequately addressed in a timely manner. I have done my best to do that here for the parts that haven't been addressed yet, and I am closing this issue as "fixed" because I think all the topics raised are either outdated, already fixed, or now have new issues open for them.

    If I missed something, please open a new issue.

contrib.social Blog FAQ Discussions
Production build 0.71.5 2024