win32/sendmail.c: Various refactorings (part 2) #20789
Conversation
And make it const too while at it.
As we don't do anything with it prior to it and we already try to reconect in SendText()
This removes the usage of a global
As we always pass it a non-NULL char* pointer. Also remove conditional check that is always true
Girgias
force-pushed
the
win-mail-part-2
branch
from
8b8e29f to
67cb1de
Compare
This allows moving out from MailConnect() the logic to determine the local host name and remove a global
Girgias
force-pushed
the
win-mail-part-2
branch
from
67cb1de to
70eb9f5
Compare
win32/sendmail.c
Outdated
| #define PHP_WIN32_MAIL_DOT_REPLACE "\n.." | ||
|
|
||
| static int SendText(const char *RPath, const char *Subject, const char *mailTo, const char *data, | ||
| static int SendText(_In_ const char *host, const char *RPath, const char *Subject, const char *mailTo, const char *data, |
There was a problem hiding this comment.
It seems this is how you'd add the __attribute__((nonnull(1))); from GCC but for Windows/MSVC, if there is another way happy to take it.
There was a problem hiding this comment.
I would need to look it up, but I don't think that's true? But it's been a while since I saw SAL.
There was a problem hiding this comment.
Well I got there by trying to navigate MS's docs... but if you find the correct thing than I'm happy to change it.
| } | ||
| } | ||
|
|
||
| /* attempt to connect with mail host */ |
There was a problem hiding this comment.
I think this commit is not very useful code motion that complicates things to be honest. If it works why change it? It's not like this is a maintainability problem.
I also don't want to diverge too much from the original wSendmail source code.
There was a problem hiding this comment.
I cannot find how the wSendmail code would even look like as everything is dead links and google has become trash and is unhelpful. So I can't even compare how much we have diverged already and if this not just "we maintain this now"
There was a problem hiding this comment.
Fair enough, although I wish reality was different, I feel like we already maintain too many parts ourselves.
There was a problem hiding this comment.
I very much agree with this, also having the whole mail functionality chucked away in a small external extension would also solve this problem of not needing to maintain this ourself :/
There was a problem hiding this comment.
I can think of two points in the past where we deviated from the original code:
-
Around 5.3 when we made use of pcre over ereg as ext/pcre became always available
-
When I many years ago fixed it to work owing IPv6 and intranet domains (specifically the HELO part)
So we have already stepped over that edge a long time ago, either way I have no opinion on the matter just some historical context
3 participants
Follow-up from #20781
Commits should be reviewed in order.
The main objective here is to get rid of some of the globals as it makes it, IMHO, unintuitive how the code flows.