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

          Attachments

            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