Uploaded image for project: 'Moodle'
  1. Moodle
  2. MDL-39552

Make the behat section of setup.php re-entrant safe

    Details

    • Testing Instructions:
      Hide

      Run, for example the @mod_lesson behat tests, and look to the php builtin server output.

      • Without the patch you will see the errors reported in the description (because of yui combo re-including part of setup.php).
      • With the patch applied, those errors aren't there anymore (and lesson tests continue passing).

      That's all.

      Show
      Run, for example the @mod_lesson behat tests, and look to the php builtin server output. Without the patch you will see the errors reported in the description (because of yui combo re-including part of setup.php). With the patch applied, those errors aren't there anymore (and lesson tests continue passing). That's all.
    • Affected Branches:
      MOODLE_25_STABLE
    • Fixed Branches:
      MOODLE_25_STABLE
    • Pull from Repository:
    • Pull Master Branch:

      Description

      This is a followup of MDL-39478... copying from there:

      Aha, just found a problem with your code in setup.php not being re-entrant (and "theme/yui_combo.php" requires all code in setup.php to be re-entrant).

      Error (in the :8000 web browser):

      [Tue May  7 18:19:23 2013] PHP Notice:  Constant BEHAT_SITE_RUNNING already defined in lib/setup.php on line 131
      [Tue May  7 18:19:23 2013] PHP Stack trace:
      [Tue May  7 18:19:23 2013] PHP   1. {main}() theme/yui_combo.php:0
      [Tue May  7 18:19:23 2013] PHP   2. require() theme/yui_combo.php:94
      [Tue May  7 18:19:23 2013] PHP   3. define() lib/setup.php:131
      

      So yui_combo.php includes setup.php again that will be run until the ABORT_AFTER_CONFIG_CANCEL point. And your behat setup code is before that ABORT_AFTER_CONFIG_CANCEL point.

      So, there are 2 solutions...

      • Or you move the behat code after the ABORT_AFTER_CONFIG_CANCEL point.
      • Or you make the behat code re-entrant, so the define is executed conditionally.

      I'm going to try the 2nd alternative...

        Gliffy Diagrams

          Issue Links

            Activity

            Hide
            stronk7 Eloy Lafuente (stronk7) added a comment -

            Sending to peer-review. I think it's safe enough...

            Show
            stronk7 Eloy Lafuente (stronk7) added a comment - Sending to peer-review. I think it's safe enough...
            Hide
            dmonllao David Monllaó added a comment - - edited

            Thanks Eloy, now I see the notices (without the patch, not anymore with the patch) pity that we can not detect that with MDL-38041. Nothing dangerous in theme/yui_combo.php between require(config.php) and require(setup.php)

            Show
            dmonllao David Monllaó added a comment - - edited Thanks Eloy, now I see the notices (without the patch, not anymore with the patch) pity that we can not detect that with MDL-38041 . Nothing dangerous in theme/yui_combo.php between require(config.php) and require(setup.php)
            Hide
            poltawski Dan Poltawski added a comment -

            Integrated and tested during integration.

            Before the patch, lots of:

            [Wed May  8 13:03:31 2013] PHP Notice:  Constant BEHAT_SITE_RUNNING already defined in /Users/danp/git/integration/lib/setup.php on line 131
            

            After none.

            One thing that I did notice is that at the end of the tests, we got this warning:

            [Wed May  8 13:12:45 2013] ::1:60409 Invalid request (Unexpected EOF)
            [Wed May  8 13:12:45 2013] ::1:60908 [200]: /mod/lesson/view.php?id=1&pageid=1
            [Wed May  8 13:12:50 2013] ::1:60910 Invalid request (Unexpected EOF)
            [Wed May  8 13:13:48 2013] ::1:53134 [303]: /mod/lesson/continue.php
            [Wed May  8 13:13:49 2013] ::1:53136 [200]: /mod/lesson/view.php?id=1&pageid=-9&outoftime=normal
            [Wed May  8 13:13:53 2013] ::1:53139 Invalid request (Unexpected EOF)
            [Wed May  8 13:13:53 2013] ::1:53138 Invalid request (Unexpected EOF)
            

            I assume its related to selenium closing the browser and the connection being interupted rather than any other problem.

            Show
            poltawski Dan Poltawski added a comment - Integrated and tested during integration. Before the patch, lots of: [Wed May 8 13:03:31 2013] PHP Notice: Constant BEHAT_SITE_RUNNING already defined in /Users/danp/git/integration/lib/setup.php on line 131 After none. One thing that I did notice is that at the end of the tests, we got this warning: [Wed May 8 13:12:45 2013] ::1:60409 Invalid request (Unexpected EOF) [Wed May 8 13:12:45 2013] ::1:60908 [200]: /mod/lesson/view.php?id=1&pageid=1 [Wed May 8 13:12:50 2013] ::1:60910 Invalid request (Unexpected EOF) [Wed May 8 13:13:48 2013] ::1:53134 [303]: /mod/lesson/continue.php [Wed May 8 13:13:49 2013] ::1:53136 [200]: /mod/lesson/view.php?id=1&pageid=-9&outoftime=normal [Wed May 8 13:13:53 2013] ::1:53139 Invalid request (Unexpected EOF) [Wed May 8 13:13:53 2013] ::1:53138 Invalid request (Unexpected EOF) I assume its related to selenium closing the browser and the connection being interupted rather than any other problem.
            Hide
            poltawski Dan Poltawski added a comment -

            Please note: I didn't run the whole suite, just the suggested mod_lesson part.

            Show
            poltawski Dan Poltawski added a comment - Please note: I didn't run the whole suite, just the suggested mod_lesson part.
            Hide
            stronk7 Eloy Lafuente (stronk7) added a comment -

            Did you think this day was not going to arrive ever?

            Your patience has been rewarded, yay, sent upstream, thanks!

            Closing...ciao

            Show
            stronk7 Eloy Lafuente (stronk7) added a comment - Did you think this day was not going to arrive ever? Your patience has been rewarded, yay, sent upstream, thanks! Closing...ciao

              People

              • Votes:
                0 Vote for this issue
                Watchers:
                1 Start watching this issue

                Dates

                • Created:
                  Updated:
                  Resolved:
                  Fix Release Date:
                  14/May/13