Uploaded image for project: 'Moodle'
  1. Moodle
  2. MDL-38613 Ensure coherence across all steps and tests
  3. MDL-39634

Remove $locator from lib/behat/behat_field_manager.php (get_node_type and get_field) is useless

    Details

      Description

      This is garbage that should be removed. That param was used before adapting get_node_type() to return a bool rather than throwing an exception. Now there is no exception because the get_field uses a default field manager if it does not find the field exacty type.

        Gliffy Diagrams

          Activity

          Hide
          poltawski Dan Poltawski added a comment -

          Hi David,

          Code is fine, but this is a tricky one - it looks like its part of the test writing API to me and could be used by external developers, so by doing this you could break third party tests.

          Therefore... we almost certainly couldn't accept this on the stable branches, unless its really really worth it?

          Another alternative might be to just to rename the variable as unused, and make it clear in phpdoc. I know this sucks for such a new api, but well, we've released and people may be using it.

          Show
          poltawski Dan Poltawski added a comment - Hi David, Code is fine, but this is a tricky one - it looks like its part of the test writing API to me and could be used by external developers, so by doing this you could break third party tests. Therefore... we almost certainly couldn't accept this on the stable branches, unless its really really worth it? Another alternative might be to just to rename the variable as unused, and make it clear in phpdoc. I know this sucks for such a new api, but well, we've released and people may be using it.
          Hide
          dmonllao David Monllaó added a comment -

          Thanks for the review Dan, as just talked here in HQ the alternatives are:

          • Rename the variable as unused, and make it clear in phpdoc (for 2.5.1 and master)
          • New function and mark the current one as deprecated in two major releases (only for master)

          I've updated the patch with the second approach, sending to peer review again (waiting for the ok to create the associated issue for the final deprecation) thanks.

          Show
          dmonllao David Monllaó added a comment - Thanks for the review Dan, as just talked here in HQ the alternatives are: Rename the variable as unused, and make it clear in phpdoc (for 2.5.1 and master) New function and mark the current one as deprecated in two major releases (only for master) I've updated the patch with the second approach, sending to peer review again (waiting for the ok to create the associated issue for the final deprecation) thanks.
          Hide
          poltawski Dan Poltawski added a comment -

          Hi David,

          Yep I like this approach too.

          I think you should backport the change to 2.5 without the debugging notice, but leave the debugging notice in as a separate commit for the integrator to make a call on. (Because there is some merit to add the debugging notice to stop people using it early in its life)

          Show
          poltawski Dan Poltawski added a comment - Hi David, Yep I like this approach too. I think you should backport the change to 2.5 without the debugging notice, but leave the debugging notice in as a separate commit for the integrator to make a call on. (Because there is some merit to add the debugging notice to stop people using it early in its life)
          Hide
          dmonllao David Monllaó added a comment -

          Thanks Dan, backported to 25. I've split the same master commit into 3:

          1. Useless argument removed and method renaming
          2. Added deprecated methods
          3. Added debugging() to deprecated methods
          Show
          dmonllao David Monllaó added a comment - Thanks Dan, backported to 25. I've split the same master commit into 3: Useless argument removed and method renaming Added deprecated methods Added debugging() to deprecated methods
          Hide
          samhemelryk Sam Hemelryk added a comment -

          Thanks David this has been integrated now.

          After mulling over it for a wee while I've integrated the 25 branch with the debugging notice.
          Given this is a support framework + a dev debug notice + its being in for only a couple of months that backporting the debugging notice is going to be friendlier than silently mapping and having people potentially implement uses of the new API without realising.

          Many thanks
          Sam

          Show
          samhemelryk Sam Hemelryk added a comment - Thanks David this has been integrated now. After mulling over it for a wee while I've integrated the 25 branch with the debugging notice. Given this is a support framework + a dev debug notice + its being in for only a couple of months that backporting the debugging notice is going to be friendlier than silently mapping and having people potentially implement uses of the new API without realising. Many thanks Sam
          Hide
          fred Frédéric Massart added a comment -

          I skipped the test on master as per David suggestion, and I am currently running the tests on 2.5 @mod, reaching 700+ without error.

          Show
          fred Frédéric Massart added a comment - I skipped the test on master as per David suggestion, and I am currently running the tests on 2.5 @mod, reaching 700+ without error.
          Hide
          fred Frédéric Massart added a comment -

          Passing the test, advised by David. Thanks!

          Show
          fred Frédéric Massart added a comment - Passing the test, advised by David. Thanks!
          Hide
          poltawski Dan Poltawski added a comment -

          Thanks for your contributions!

          _main:
          @ BB#0:
                  push    {r7, lr}
                  mov     r7, sp
                  sub     sp, #4
                  movw    r0, :lower16:(L_.str-(LPC0_0+4))
                  movt    r0, :upper16:(L_.str-(LPC0_0+4))
          LPC0_0:
                  add     r0, pc
                  bl      _printf
                  movs    r1, #0
                  movt    r1, #0
                  str     r0, [sp]                @ 4-byte Spill
                  mov     r0, r1
                  add     sp, #4
                  pop     {r7, pc}
           
                  .section        __TEXT,__cstring,cstring_literals
          L_.str:                                 @ @.str
                  .asciz   "This code is now upstream!"
          

          Show
          poltawski Dan Poltawski added a comment - Thanks for your contributions! _main: @ BB#0: push {r7, lr} mov r7, sp sub sp, #4 movw r0, :lower16:(L_.str-(LPC0_0+4)) movt r0, :upper16:(L_.str-(LPC0_0+4)) LPC0_0: add r0, pc bl _printf movs r1, #0 movt r1, #0 str r0, [sp] @ 4-byte Spill mov r0, r1 add sp, #4 pop {r7, pc}   .section __TEXT,__cstring,cstring_literals L_.str: @ @.str .asciz "This code is now upstream!"

            People

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

              Dates

              • Created:
                Updated:
                Resolved:
                Fix Release Date:
                8/Jul/13