[MDL-27475] Modify start/dispatch/end notifications to be called in correct order - Moodle Tracker

Details

Type: Sub-task
Status: Closed
Priority: Major
Resolution: Fixed
Affects Version/s: 2.1
Fix Version/s: 2.1
Component/s: Backup
Labels:
- pullweek-2011-20
- triaged

Testing Instructions:
Hide

Run the unit tests (Admin -> Developer -> Unit tests) by specifying "backup/util/xml/parser".

TEST: All the tests pass ok

TEST: The number of tests is, at least 216

(note this is one 2.1dev only integration)
Show
Run the unit tests (Admin -> Developer -> Unit tests) by specifying "backup/util/xml/parser". TEST: All the tests pass ok TEST: The number of tests is, at least 216 (note this is one 2.1dev only integration)
Difficulty:
Easy
Affected Branches:
MOODLE_21_STABLE
Fixed Branches:
MOODLE_21_STABLE
Pull from Repository:
git://github.com/stronk7/moodle.git
Pull Master Branch:
start_end_notifications
Pull Master Diff URL:
https://github.com/stronk7/moodle/compare/master...start_end_notifications

Description

It has been detected that the new start/end notifications were being dispatched in incorrect order some times.

It was caused because we were using some parser internals to perform the notification. So the idea is to move the notifications to proper place, unrelated with parser and "ordered" for any reader (human or machine).

Gliffy Diagrams

Issue Links

has been marked as being related by

MDL-27594 XML start/end parsing modifications correct order on empty paths

Closed

MDL-27693 Yet another issue with the order of start/process/end handlers in the progressive parser

Closed

Activity

Ascending order - Click to sort in descending order

Hide

Permalink

Sam Hemelryk added a comment - 16/May/11 10:08 AM

Thanks Eloy - integrated now

Show

Sam Hemelryk added a comment - 16/May/11 10:08 AM Thanks Eloy - integrated now

Hide

Permalink

Helen Foster added a comment - 17/May/11 9:08 PM

Tested on http://qa.moodle.net/

1/1 test cases complete: 216 passes, 0 fails and 0 exceptions.

Show

Helen Foster added a comment - 17/May/11 9:08 PM Tested on http://qa.moodle.net/ 1/1 test cases complete: 216 passes, 0 fails and 0 exceptions.

Hide

Permalink

Eloy Lafuente (stronk7) added a comment - 18/May/11 9:14 PM

Closing this, it's already part of upstream, thanks!

Show

Eloy Lafuente (stronk7) added a comment - 18/May/11 9:14 PM Closing this, it's already part of upstream, thanks!

Hide

Permalink

David Mudrak added a comment - 18/May/11 11:09 PM

Eloy, I can confirm the order is correct now. But I have noticed that now, the notify_path_start() is not executed if the path does not contain any final elements. Eg for

<a>

<b>

    <c>VALUE</c>

  </b>

</a>

the first start notification would be for "b started" because <a> contains just subpaths, not final elements. However, the notify_path_end() would be triggered for </a> normally. I just want to make sure this is expected/needed. If so, I will stop using notify_path_start() at all because I can't rely on it being executed...

Show

David Mudrak added a comment - 18/May/11 11:09 PM Eloy, I can confirm the order is correct now. But I have noticed that now, the notify_path_start() is not executed if the path does not contain any final elements. Eg for <a> <b> <c>VALUE</c> </b> </a> the first start notification would be for "b started" because <a> contains just subpaths, not final elements. However, the notify_path_end() would be triggered for </a> normally. I just want to make sure this is expected/needed. If so, I will stop using notify_path_start() at all because I can't rely on it being executed...

Hide

Permalink

David Mudrak added a comment - 18/May/11 11:11 PM

... just thinking, is it related to the issue with the head-sub-branches? (that is when an element contents starts with a subbranch?)

Show

David Mudrak added a comment - 18/May/11 11:11 PM ... just thinking, is it related to the issue with the head-sub-branches? (that is when an element contents starts with a subbranch?)

Hide

Permalink

David Mudrak added a comment - 19/May/11 9:45 AM

Hmm and I seem to have yet another problem with the order. Let us have a structure like this (real example):

  <MOODLE_BACKUP>

    <COURSE>

      <SECTION>

        <ID>436</ID>

        <MODS>

          <MOD>

            <ID>250</ID>

            <ROLES_OVERRIDES>

            </ROLES_OVERRIDES>

          </MOD>

        </MODS>

      </SECTION>

    </COURSE>

  </MOODLE_BACKUP>

and let us have the following paths registered:

/MOODLE_BACKUP/COURSE/SECTIONS/SECTION

/MOODLE_BACKUP/COURSE/SECTIONS/SECTION/MODS/MOD

/MOODLE_BACKUP/COURSE/SECTIONS/SECTION/MODS/MOD/ROLES_OVERRIDES

Then the event handler for the end of the .../ROLES_OVERRIDES path is executed first before the processing method of the .../MOD. Therefore I am unable to parse data progressively because I have no way how to get to the information of MOD's ID from the ROLES_OVERRIDES. The only way at the moment is to stash ROLES_OVERRIDES in a temporarily memory and actually process it (create a file in this case) later.

IMHO the MOD's processing and event handling should happen first, no?
when such a structure is parsed

Show

David Mudrak added a comment - 19/May/11 9:45 AM Hmm and I seem to have yet another problem with the order. Let us have a structure like this (real example): <MOODLE_BACKUP> <COURSE> <SECTION> <ID>436</ID> <MODS> <MOD> <ID>250</ID> <ROLES_OVERRIDES> </ROLES_OVERRIDES> </MOD> </MODS> </SECTION> </COURSE> </MOODLE_BACKUP> and let us have the following paths registered: /MOODLE_BACKUP/COURSE/SECTIONS/SECTION /MOODLE_BACKUP/COURSE/SECTIONS/SECTION/MODS/MOD /MOODLE_BACKUP/COURSE/SECTIONS/SECTION/MODS/MOD/ROLES_OVERRIDES Then the event handler for the end of the .../ROLES_OVERRIDES path is executed first before the processing method of the .../MOD. Therefore I am unable to parse data progressively because I have no way how to get to the information of MOD's ID from the ROLES_OVERRIDES. The only way at the moment is to stash ROLES_OVERRIDES in a temporarily memory and actually process it (create a file in this case) later. IMHO the MOD's processing and event handling should happen first, no? when such a structure is parsed

Hide

Permalink

Eloy Lafuente (stronk7) added a comment - 19/May/11 3:56 PM

Yes, 100%.

That is the sort of things I supposedly tested with helper_check_notifications_order_integrity() in parser tests, grrr. Going to check with your example...

Show

Eloy Lafuente (stronk7) added a comment - 19/May/11 3:56 PM Yes, 100%. That is the sort of things I supposedly tested with helper_check_notifications_order_integrity() in parser tests, grrr. Going to check with your example...

Hide

Permalink

Eloy Lafuente (stronk7) added a comment - 19/May/11 4:20 PM

Side comment, just to be 100% sure... in the example above... have you noticed that the registered paths and the XML doesn't match? I mean, there is no "SECTIONS" tree in the XML file.

I've added it here, to my tests, to continue investigating... just commenting in case that's related with your problem.

Ciao

Show

Eloy Lafuente (stronk7) added a comment - 19/May/11 4:20 PM Side comment, just to be 100% sure... in the example above... have you noticed that the registered paths and the XML doesn't match? I mean, there is no "SECTIONS" tree in the XML file. I've added it here, to my tests, to continue investigating... just commenting in case that's related with your problem. Ciao

Hide

Permalink

Eloy Lafuente (stronk7) added a comment - 19/May/11 4:22 PM - edited

Wow, this is crazy, I get these notifications:

Ordered allnotifs:

    [0] => start:/MOODLE_BACKUP/COURSE/SECTIONS/SECTION

    [1] => process:/MOODLE_BACKUP/COURSE/SECTIONS/SECTION

    [2] => end:/MOODLE_BACKUP/COURSE/SECTIONS/SECTION/MODS/MOD/ROLES_OVERRIDES

    [3] => start:/MOODLE_BACKUP/COURSE/SECTIONS/SECTION/MODS/MOD

    [4] => process:/MOODLE_BACKUP/COURSE/SECTIONS/SECTION/MODS/MOD

    [5] => end:/MOODLE_BACKUP/COURSE/SECTIONS/SECTION/MODS/MOD

    [6] => end:/MOODLE_BACKUP/COURSE/SECTIONS/SECTION

So, obviously, something is wrong there... going to debug a bit....

Show

Eloy Lafuente (stronk7) added a comment - 19/May/11 4:22 PM - edited Wow, this is crazy, I get these notifications: Ordered allnotifs: [0] => start:/MOODLE_BACKUP/COURSE/SECTIONS/SECTION [1] => process:/MOODLE_BACKUP/COURSE/SECTIONS/SECTION [2] => end:/MOODLE_BACKUP/COURSE/SECTIONS/SECTION/MODS/MOD/ROLES_OVERRIDES [3] => start:/MOODLE_BACKUP/COURSE/SECTIONS/SECTION/MODS/MOD [4] => process:/MOODLE_BACKUP/COURSE/SECTIONS/SECTION/MODS/MOD [5] => end:/MOODLE_BACKUP/COURSE/SECTIONS/SECTION/MODS/MOD [6] => end:/MOODLE_BACKUP/COURSE/SECTIONS/SECTION So, obviously, something is wrong there... going to debug a bit....

Hide

Permalink

Eloy Lafuente (stronk7) added a comment - 19/May/11 4:30 PM - edited

Ah, my friend, I think I've got it. Problem happens because ROLES_OVERRIDES is empty, afaik. I've added one simple ID tag below it and then I get this (correct) list of notifications:

Ordered allnotifs:

    [0] => start:/MOODLE_BACKUP/COURSE/SECTIONS/SECTION

    [1] => process:/MOODLE_BACKUP/COURSE/SECTIONS/SECTION

    [2] => start:/MOODLE_BACKUP/COURSE/SECTIONS/SECTION/MODS/MOD

    [3] => process:/MOODLE_BACKUP/COURSE/SECTIONS/SECTION/MODS/MOD

    [4] => start:/MOODLE_BACKUP/COURSE/SECTIONS/SECTION/MODS/MOD/ROLES_OVERRIDES

    [5] => process:/MOODLE_BACKUP/COURSE/SECTIONS/SECTION/MODS/MOD/ROLES_OVERRIDES

    [6] => end:/MOODLE_BACKUP/COURSE/SECTIONS/SECTION/MODS/MOD/ROLES_OVERRIDES

    [7] => end:/MOODLE_BACKUP/COURSE/SECTIONS/SECTION/MODS/MOD

    [8] => end:/MOODLE_BACKUP/COURSE/SECTIONS/SECTION

So, at some point, when one path is "empty", we are skipping too many calls... debugging...

Show

Eloy Lafuente (stronk7) added a comment - 19/May/11 4:30 PM - edited Ah, my friend, I think I've got it. Problem happens because ROLES_OVERRIDES is empty, afaik. I've added one simple ID tag below it and then I get this (correct) list of notifications: Ordered allnotifs: [0] => start:/MOODLE_BACKUP/COURSE/SECTIONS/SECTION [1] => process:/MOODLE_BACKUP/COURSE/SECTIONS/SECTION [2] => start:/MOODLE_BACKUP/COURSE/SECTIONS/SECTION/MODS/MOD [3] => process:/MOODLE_BACKUP/COURSE/SECTIONS/SECTION/MODS/MOD [4] => start:/MOODLE_BACKUP/COURSE/SECTIONS/SECTION/MODS/MOD/ROLES_OVERRIDES [5] => process:/MOODLE_BACKUP/COURSE/SECTIONS/SECTION/MODS/MOD/ROLES_OVERRIDES [6] => end:/MOODLE_BACKUP/COURSE/SECTIONS/SECTION/MODS/MOD/ROLES_OVERRIDES [7] => end:/MOODLE_BACKUP/COURSE/SECTIONS/SECTION/MODS/MOD [8] => end:/MOODLE_BACKUP/COURSE/SECTIONS/SECTION So, at some point, when one path is "empty", we are skipping too many calls... debugging...

Hide

Permalink

Eloy Lafuente (stronk7) added a comment - 23/May/11 7:19 AM

The solution for this case has been requested for integration @ ~~MDL-27594~~, plz follow it there, thanks!

Show

Eloy Lafuente (stronk7) added a comment - 23/May/11 7:19 AM The solution for this case has been requested for integration @ MDL-27594 , plz follow it there, thanks!

People

Assignee:

Eloy Lafuente (stronk7)

Reporter:

Eloy Lafuente (stronk7)

Integrator:

Sam Hemelryk

Tester:

Helen Foster

Participants:

David Mudrak, Eloy Lafuente (stronk7), Helen Foster, Sam Hemelryk

Votes:

0 Vote for this issue

Watchers:

0 Start watching this issue

Dates

Created:

12/May/11 1:56 AM

Updated:

02/Jun/11 8:31 AM

Resolved:

18/May/11 9:14 PM

Agile

View on Board