This is a followup to #1508104-5: [PATCH] Replace dynamically generated class with delegation.

Now that generated classes are eliminated from the files directory, the risk of ruining a site because of missing mailsystem classes as a result of moving configuration around is much lower than before. Therefore it is not necessary anymore to try to fix the registry and the mail_system variable on each page request via hook_init. Rather hook_requerements is implemented in order to notify the administrator whenever the mail-configuration is broken.

In order to give site-builders a chance on customizing which classes are inspected when discovering implementors of MailSystemInterface two new variables are introduced: mailsystem_get_classes_whitelist and mailsystem_get_classes_blacklist. The default behaviour is preserved (classname must end in "MailSystem" or the filename must contain the string ".mail."). Also the PHP function class_implements is now used when determining wheter a candidate class implements the MailSystemInterface. This goes without instantiation and therefore will hopefully lead to less trouble with third party modules.

CommentFileSizeAuthor
#6 0004-Remove-hook_init-completely_6.patch420 bytesmrfelton
0004-Remove-hook_init-completely.patch1 KBznerol
0003-Rewrite-mailsystem_get_classes.patch7.28 KBznerol
0002-Add-hook_requirements-implementation-which-checks-fo.patch2.2 KBznerol
0001-Move-mailsystem_admin_read_settings-to-mailsystem_re.patch2.94 KBznerol

Comments

pillarsdotnet’s picture

pillarsdotnet commented

thanks; will review when I can.

sbrattla’s picture

sbrattla commented

Now that generated classes are eliminated from the files directory [...]

Which version are you using? I just downloaded 7.x-2.34 which makes my entire site go down when I activate it. A quick look at the logs gives me the following :

[Mon May 14 22:35:01 2012] [warn] [client 31.222.158.70] mod_fcgid: stderr: PHP Fatal error:  require_once(): Failed opening required '/var/www/testsite/public_html//var/www/testsite/files/mailsystem/DefaultMailSystem__SWIFTMailSystem.mail.inc' (include_path='.:/usr/share/php:/usr/share/pear') in /var/www/testsite/public_html/includes/bootstrap.inc on line 3069

It appears that the version I've downloaded is still generating classes which are being put into the files directory. Somehow, that doesn't really work very well for me.

pillarsdotnet’s picture

pillarsdotnet commented

You are correct. The patch from this issue has not been rolled into production yet.

mrfelton’s picture

mrfelton commented

This is great. Seeing massive performance improvements with this.

mrfelton’s picture

mrfelton commented
Status: Needs review » Needs work

Should patch 004 really be removing the following from hook_init():


// @todo Remove this when issue #299138 gets resolved.
if (!function_exists('mailsystem_html_to_text')) {
  module_load_include('inc', 'mailsystem', 'html_to_text');
}

Issue #299138: Improve \Drupal\Core\Utility\Mail::htmlToText() is not yet resolved. Removing this breaks the htmlmail module.

mrfelton’s picture

mrfelton commented
Status: Needs work » Needs review
StatusFileSize
new 0004-Remove-hook_init-completely_6.patch420 bytes

Here is a version that removes the call to mailsystem_get_classes(), but leaves the mailsystem_html_to_text function definition.

znerol’s picture

znerol commented

Removing the whole hook_init implementation goes hand in hand with #1538148: [PATCH] Extract mailsystem_html_to_text into a submodule . Actually just loading this include file does not result in any additional/improved functionality for the drupal_html_to_text function. Rather it just frees contrib developers to load the file on their own which is IMHO not a good idea. Especially because hook_init should be avoided if possible because of performance reasons.

pillarsdotnet’s picture

pillarsdotnet commented

Noted.

pillarsdotnet’s picture

pillarsdotnet commented
Version: 7.x-2.x-dev » 7.x-3.0-alpha1
Assigned: Unassigned » pillarsdotnet
Status: Needs review » Fixed

Committed to 7.x-3.0-alpha1

Status: Fixed » Closed (fixed)

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