Hooks returning None should make smtp.py behave like nothing happened.

[cnicodeme]

When hooks are called for each steps of the SMTP call, the code check if the MISSING value is returned, which is an instance of object().

I'm curious to know why.

From my perspective, if a call to a hook returns None, the code should continue like if there were no hooks at all. The reason behind that is if no response is returned from the hook, the server still has to return a status, and the default one is accepted by the developer.

Moreover, if a hook exists, it MUST implement the logics presents in smtp.py when no hooks exists. Let me explain:

Here's the line for HELO commands

If there are no hooks, here's what's happening:

        status = await self._call_handler_hook('HELO', hostname)
        if status is MISSING:
            self.session.host_name = hostname
            status = '250 HELP'

But as soon as I add a HELO handler, the status won't be MISSING, which means I have to implement the two consequent lines in my own code, by copy/pasting it. That's not good.

Instead, if the smtp.py code does the following:

        status = await self._call_handler_hook('HELO', hostname)
        # This expect that the self._call_handler_hook returns also None in case no handler was found
        if status is None:
            self.session.host_name = hostname
            status = '250 HELP'

My handle can return None, saying "everything is good" like all hooks behave, and smtp.py will do the intended work.

But maybe I'm missing a specific reason, so I'd be curious to know.

Otherwise, happy to submit a PR if you want!

[pepoluan]

Well, you can return MISSING from your handler. MISSING is after all an object importable from aiosmtpd.smtp

Like such:

from aiosmtpd.smtp import MISSING

class MyHandler:

    async def handle_HELO(...):
        ....
        return MISSING

But I agree, this is an awkward API with some surprises. I'll see what I can do.

[cnicodeme]

The idea is that it does not make sense to return an Object() just for the sake of returning something that means nothing, and having to import it to do so.

Semantically speaking, None represents MISSING, which is a standard value ;)

That's what the PR was about :)

[pepoluan]

Understood. I may have misconstrued what I meant to say in my comment there, my bad.

Changing the behavior might have extensive repercussions, though, for people who already got 'bitten' by this awkward interaction and adapted, so any changes will need to be vetted & tested carefully.

Besides, there seems to be a plan to totally rewrite how EHLO handler works ( #155 ), and changes there can -- and should -- trickle down to the HELO handler.

I'd personally wait until #155 is resolved before trying to fix this.

EDIT: Ahaha I just noticed that #155 was also yours.

[cnicodeme]

Oh yes, ok, said like that it makes total sense!

I saw your suggestion on #155 and it's a good way to allow both MISSING and None, then put a DeprecationWarning in a few releases later, and then finally stop it? That way, there is no breaking updates :)

[pepoluan]

Oh yes, ok, said like that it makes total sense!

I saw your suggestion on #155 and it's a good way to allow both MISSING and None, then put a DeprecationWarning in a few releases later, and then finally stop it? That way, there is no breaking updates :)

You mean #157 ? :D

I don't think we need to put DeprecationWarning, just rewrite the smtp_* methods to act on None as if it's MISSING; the latter will still be returned by SMTP._call_handlerhook if handler does not implement the relevant `handle*` method,.

I'll earmark this for 1.3 as doing so is not breaking.

[cnicodeme]

My bad, I did meant #157 ;)

[pepoluan]

I'll create a PR after the 2 PRs for 1.2.4 -- kinda urgent-ish bugfixes -- gets merged.

[pepoluan]

I actually have created a branch for this issue: https://github.com/pepoluan/aiosmtpd/tree/treat-none-missing

Originally thought that this can be pushed into 1.4.

But after some thinking, I've decided to push this into 2.0 instead, because the changes I made potentially break existing software, and we can't have that before 2.0.

In the meanwhilst, I can keep tuning this branch.

So, sorry for everyone wanting to see this implemented in the 1.x series: It just won't happen. You'll have to import MISSING and use that, instead.

Feel free to explore my strategy of solving this issue in the branch above. Criticism & discussion is of course welcome.