Permalink
Browse files

Fix wrong email gateway warning

The warning was shown when the email gateway settings is not prefixed with 'email_'
  • Loading branch information...
nitriques committed Nov 23, 2017
1 parent 3be0099 commit b7bbcdec9687ee0348276bb8bf31b3d7ad14c7e4
Showing with 3 additions and 1 deletion.
  1. +3 −1 lib/class.ABF.php
View
@@ -700,7 +700,9 @@ public function getEmailSettings()
{
$emailGateway = Symphony::Configuration()->get('default_gateway', 'email');
$emailSettings = Symphony::Configuration()->get('email_'.$emailGateway);
if (empty($emailSettings)) {
$emailSettings = Symphony::Configuration()->get($emailGateway);
}
return is_array($emailSettings) ? $emailSettings : null;
}

14 comments on commit b7bbcde

@michael-e

This comment has been minimized.

Show comment
Hide comment
@michael-e

michael-e Nov 24, 2017

Contributor

What are you trying to fix here? In which cases is the setting not prefixed? (I looked at my installations, and I have not found any un-prefixed settings.)

Contributor

michael-e replied Nov 24, 2017

What are you trying to fix here? In which cases is the setting not prefixed? (I looked at my installations, and I have not found any un-prefixed settings.)

@nitriques

This comment has been minimized.

Show comment
Hide comment
@nitriques

nitriques Nov 24, 2017

Member

have not found any un-prefixed setting

I've started to work on an extension that offers a lot more than an email gateway. I had the pesky warning that I did not had an email-gateway configured even though the can should work (I do have a 'from_address').

Is it causing a problem ?

Member

nitriques replied Nov 24, 2017

have not found any un-prefixed setting

I've started to work on an extension that offers a lot more than an email gateway. I had the pesky warning that I did not had an email-gateway configured even though the can should work (I do have a 'from_address').

Is it causing a problem ?

@michael-e

This comment has been minimized.

Show comment
Hide comment
@michael-e

michael-e Nov 24, 2017

Contributor

Well, the function is called getEmailSettings, and the doc comment says @return The email settings for the default Email Gateway. It's hard to understand why the "default email gateway" should ever not be prefixed with email_.

I don't know if this will cause problems. But potentially, isn't changing or weakening conventions (here: namespaces) a problem in itself? :-)

Contributor

michael-e replied Nov 24, 2017

Well, the function is called getEmailSettings, and the doc comment says @return The email settings for the default Email Gateway. It's hard to understand why the "default email gateway" should ever not be prefixed with email_.

I don't know if this will cause problems. But potentially, isn't changing or weakening conventions (here: namespaces) a problem in itself? :-)

@nitriques

This comment has been minimized.

Show comment
Hide comment
@nitriques

nitriques Nov 24, 2017

Member

It's hard to understand why the "default email gateway" should ever not be prefixed with email_

Simply because the settings are shared across multiple bits and pieces (mainly the email gateway and events).

Member

nitriques replied Nov 24, 2017

It's hard to understand why the "default email gateway" should ever not be prefixed with email_

Simply because the settings are shared across multiple bits and pieces (mainly the email gateway and events).

@michael-e

This comment has been minimized.

Show comment
Hide comment
@michael-e

michael-e Nov 24, 2017

Contributor

Still, I don't like this fix. It provides a fallback for getting email settings if someone (or some extension) has not followed the convention. I don't say that it's always possible to follow conventions, but this fix is somewhat "isolated"; it's only in this single extension, and other extensions will not see it, so they won't work with settings missing the namespace. All in all the solution may be a "quick fix", but it is not "stable" nor future proof.

Contributor

michael-e replied Nov 24, 2017

Still, I don't like this fix. It provides a fallback for getting email settings if someone (or some extension) has not followed the convention. I don't say that it's always possible to follow conventions, but this fix is somewhat "isolated"; it's only in this single extension, and other extensions will not see it, so they won't work with settings missing the namespace. All in all the solution may be a "quick fix", but it is not "stable" nor future proof.

@nitriques

This comment has been minimized.

Show comment
Hide comment
@nitriques

nitriques Nov 24, 2017

Member

What should I do then ? (renaming the settings group is not an option)

Member

nitriques replied Nov 24, 2017

What should I do then ? (renaming the settings group is not an option)

@michael-e

This comment has been minimized.

Show comment
Hide comment
@michael-e

michael-e Nov 24, 2017

Contributor

What should I do then ?

I was afraid that you would ask this. This is hard, at 10:40 p.m.

renaming the settings group is not an option

Really sure? Couldn't your new extension work with several settings groups? Email settings and event settings?

If that is impossible or makes things worse, there may not be a perfect solution (as in: "Michael really likes this"). But maybe you consider adding another function which gets the settings of "the special extension". Now when you need the settings, you could attempt to get the email settings first, and if this fails, you call the other function with the "special settings". This would at least keep the getEmailSettings function clean. Definitely not a big improvement, but maybe a little one.

It all depends on what you actually do with your new extension, so I can't say any more. (We all know that naming things is one out of two hard things in programming.)

Contributor

michael-e replied Nov 24, 2017

What should I do then ?

I was afraid that you would ask this. This is hard, at 10:40 p.m.

renaming the settings group is not an option

Really sure? Couldn't your new extension work with several settings groups? Email settings and event settings?

If that is impossible or makes things worse, there may not be a perfect solution (as in: "Michael really likes this"). But maybe you consider adding another function which gets the settings of "the special extension". Now when you need the settings, you could attempt to get the email settings first, and if this fails, you call the other function with the "special settings". This would at least keep the getEmailSettings function clean. Definitely not a big improvement, but maybe a little one.

It all depends on what you actually do with your new extension, so I can't say any more. (We all know that naming things is one out of two hard things in programming.)

@nitriques

This comment has been minimized.

Show comment
Hide comment
@nitriques

nitriques Nov 24, 2017

Member

Email settings and event settings?

It's the same values!

Maybe I could be only the 'from_address' separated ...

Go to sleep. We'll figure this out sleeping ;)

Member

nitriques replied Nov 24, 2017

Email settings and event settings?

It's the same values!

Maybe I could be only the 'from_address' separated ...

Go to sleep. We'll figure this out sleeping ;)

@michael-e

This comment has been minimized.

Show comment
Hide comment
@michael-e

michael-e Nov 24, 2017

Contributor

We'll figure this out sleeping

Now that is a good idea indeed! :-)

See you!

Contributor

michael-e replied Nov 24, 2017

We'll figure this out sleeping

Now that is a good idea indeed! :-)

See you!

@nitriques

This comment has been minimized.

Show comment
Hide comment
@nitriques
Member

nitriques replied Nov 24, 2017

:)

@michael-e

This comment has been minimized.

Show comment
Hide comment
@michael-e

michael-e Nov 25, 2017

Contributor

Another try:

I assume that you want your new extension to be "as compatible as possible" with other extensions. Well, won't other extensions always assume that email prefs are prefixed with email_?

So for the new extension, why not think about writing to multiple settings groups synchronously – could this make sense? Write to email_ what belongs there, and write (partially) the same values to another array (which means a different context). If you ensure that the values are always written to both arrays synchronously, couldn't this be the most bullet-proof solution?

I admit that the answer really depends on how your new extension works. We should keep in mind someone editing the config file manually, with useful values, carefully, but without knowledge of the inner workings of installed extensions. What can break? What will break?

Contributor

michael-e replied Nov 25, 2017

Another try:

I assume that you want your new extension to be "as compatible as possible" with other extensions. Well, won't other extensions always assume that email prefs are prefixed with email_?

So for the new extension, why not think about writing to multiple settings groups synchronously – could this make sense? Write to email_ what belongs there, and write (partially) the same values to another array (which means a different context). If you ensure that the values are always written to both arrays synchronously, couldn't this be the most bullet-proof solution?

I admit that the answer really depends on how your new extension works. We should keep in mind someone editing the config file manually, with useful values, carefully, but without knowledge of the inner workings of installed extensions. What can break? What will break?

@nitriques

This comment has been minimized.

Show comment
Hide comment
@nitriques

nitriques Dec 4, 2017

Member

I assume that you want your new extension to be "as compatible as possible" with other extensions.

I do not. I want that extension to offer everything Symphony can offer: events, data-sources and email gateway.

So for the new extension, why not think about writing to multiple settings groups synchronously – could this make sense?

It would, kinda. But I would still use data from multiple groups in the email gateway.

If you ensure that the values are always written to both arrays synchronously, couldn't this be the most bullet-proof solution?

No because we do:

  1. commit the config.php file
  2. edit it manually
Member

nitriques replied Dec 4, 2017

I assume that you want your new extension to be "as compatible as possible" with other extensions.

I do not. I want that extension to offer everything Symphony can offer: events, data-sources and email gateway.

So for the new extension, why not think about writing to multiple settings groups synchronously – could this make sense?

It would, kinda. But I would still use data from multiple groups in the email gateway.

If you ensure that the values are always written to both arrays synchronously, couldn't this be the most bullet-proof solution?

No because we do:

  1. commit the config.php file
  2. edit it manually
@michael-e

This comment has been minimized.

Show comment
Hide comment
@michael-e

michael-e Dec 5, 2017

Contributor

Thanks for your explanation. I am afraid that — based on what I know about your issue — I can't propose anything else.

But simply looking at it… The addition to the ABF extension feels so "unmotivated". The rationale behind it is hard to see. Maybe you could at least add a useful comment? (Something like "Fallback for extensions that provide gateways without prefixing the gateway settings" or similar.)

Contributor

michael-e replied Dec 5, 2017

Thanks for your explanation. I am afraid that — based on what I know about your issue — I can't propose anything else.

But simply looking at it… The addition to the ABF extension feels so "unmotivated". The rationale behind it is hard to see. Maybe you could at least add a useful comment? (Something like "Fallback for extensions that provide gateways without prefixing the gateway settings" or similar.)

@nitriques

This comment has been minimized.

Show comment
Hide comment
@nitriques

nitriques Dec 29, 2017

Member

See b7bd001

The extension that needed got canceled, so will poke with this next time it is needed. Thanks @michael-e

Member

nitriques replied Dec 29, 2017

See b7bd001

The extension that needed got canceled, so will poke with this next time it is needed. Thanks @michael-e

Please sign in to comment.