Observer Method Return Value Alignment: Always Void

[Sdfendor]

Description (*)

Observer methods called by the event system, analogous to controller action methods, return nothing (return type void). Any returned value would be discarded (method are calls in event mainloop by $object->$method($observer);):

While a returned value does no harm and is just discarded, it can't be used and it's useless to return anything, because there is no access to this data. To transport data out of the observer method the observer object/event object within it can be filled and then used after a successful dispatchEvent call. I therefore think all observer methods currently active (having observers defined in config.xml files) should be aligned in their return type and return behavior not least to avoid giving bad examples in the core for developers working with observers which would be confusing and counterproductive.

This PR:

1. Removes return values from all observers defined in the various config.xml, but of course keeping all early returns (return;)
2. Removes redundant phpdoc annotations but also added mainly throws annotations
3. Added a todo for a really long observer method (>200 LOC) to refactor at some future time
4. Small fixes in a few conditionals

Contribution checklist (*)

• [x] Pull request has a meaningful description of its purpose
• [x] All commits are accompanied by meaningful commit messages
• [x] All automated tests passed successfully (all builds are green)

[sreichel]

Not sure about this. Can you please keep the docblocks? Tools like phpDocumentor use them.

Observer returns can be used outside, see

https://github.com/AOEpeople/Aoe_Scheduler/blob/master/app/code/community/Aoe/Scheduler/Model/Schedule.php#L229-L259

Maybe it would be better to add missing return $this for more consistent code?

[Sdfendor]

1.) As far as I understand it phpDocumentor uses php's own typing just fine. I only removed redundant phpdoc content. So e.g. I removed it here:

because the $observer parameter is already typed in code. But I didn't remove it where there is no typing in code because there the phpdoc is of interest. I also wouldn't have removed it if there would be meaningful text after the type declaration of a parameter, but there wereno such cases. I didn't replaced the @return annotations with void though, if that is what you mean?

2.) I'm aware of cron jobs and I'm aware of Aoe_Scheduler and I read the code you marked but I can't say that I understand what this has to do with the event/observer system. As I understand it crons are defined differently crontab/jobs in config.xml and are called differently. AOE scheduler calls the crons in line 217 of the file shown: and takes the return value and uses it, which is fine I guess. All the dispatching that is done afterwards in regards to the contents of $messages does look like ordinary events. As I have shown in the PR description the event system doesn't do that, but simply calls the method within the object as defined in an observer in config.xml. I did not change anything regarding crons as far as I am aware.

Can you please elaborate where you see the connection here? Do I need to look deeper into any method of the shown code for that?

[sreichel]

1.) phpDocumentor ... tested. You're right. Redunant docblocks are not needed. Maybe I had that in mind from php5 ...

2.) cronjobs also accept observer-methods as callback ...

app/code/core/Mage/CatalogRule/etc/config.xml

       <jobs>
            <catalogrule_apply_all>
                <schedule>
                    <cron_expr>0 1 * * *</cron_expr>
                </schedule>
                <run>
                    <model>catalogrule/observer::dailyCatalogUpdate</model>
                </run>
            </catalogrule_apply_all>
        </jobs>

Changed for test ... Mage_CatalogRule_Model_Observer::dailyCatalogUpdate

    public function dailyCatalogUpdate($observer)
    {
        /** @var Mage_CatalogRule_Model_Rule $model */
        $model = Mage::getSingleton('catalogrule/rule');
        $model->applyAll();

        return 'test'; // added
        return $this;
    }

[Sdfendor]

@sreichel Regarding cronjobs: Due to the nature of the cronjob feature arbitrary methods are possible to be executed and Aoe_Scheduler evaluates returned information. I see your point here, observer methods are no exception to this. But I have to say I'm not sure if this isn't more of a side effect after all. This feature could be useful but insn't in any case I changed in this PR, let me explain.

Observer methods have generally the Varien_Event_Observer $observer parameter if they should transport data mainly in- but also outside the context of the method. Due to being a Varien_Object arbirtrary payload can be added and this is utilized in various of the those methods. In contrast no core observer method being changed in this PR uses its object ($this) to transport anything, no data is set, they only return it. Maybe this is due to convention of fluent interface that is whitespread within Open Mage, but that is besides the point. So if this is the case where is the information gained here (opposite to data)? In the code shown if the returned value is an object, the class is retrieved and printed: $messages = get_class($messages);. So let's say one uses an observer method within a cronjob as you've shown in the example code, then the data within message is Mage_CatalogRule_Model_Observer. I wouldn't call this information.

I didn't remove any code that used $this in a meaningful way where one could assume that it happened in a deliberate way to benefit from the feature of Aoe_Scheduler. That's my point. I still think this PR removes bloat.

Besides that no one is prevented from using any custom observer method in the way you've shown, if there is specific use case for it. PR contents and the Aoe_Schedule feature therefore aren't mutually exclusive.

[fballiano]

solved conflict

[fballiano]

switching to draft because of the many conflicts

[fballiano]

is somebody willing to take this over and fix the conflicts?

[fballiano]

I sorta tested it (in the sense that I couldn't find anything breaking after applying this PR) so maybe we should consider it. I don't know, it's a delicate thing to change return types, it should probably be considered breaking-change and moved to next, but it is true that returning this in an observer it's not that clean and should be avoided so...

I don't know. I'd probably vote to merge it but in the next branch.