Moodle
  1. Moodle
  2. MDL-15040

<br/> should be replaced by <br />

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Trivial Trivial
    • Resolution: Not a bug
    • Affects Version/s: 1.9.1, 1.9.2, 1.9.3, 2.0.6, 2.1, 2.2
    • Fix Version/s: None
    • Component/s: General
    • Labels:
      None
    • Rank:
      7007

      Description

      in data/lib.php at row 1183 you can find <br/> instead of <br />. This breaks XHTML strict.

        Activity

        Daniele Cordella created issue -
        Hide
        Daniele Cordella added a comment - - edited

        oh oooooooh!!!!!

        8-O

        cd /Applications/MAMP/htdocs/moodle19
        grep -rn '<br/>' *

        201 rows of output!!!!!!

        ( The occurrences number 111 is the one I rise up before!!!!
        He he, coherence preserved!!! )

        Show
        Daniele Cordella added a comment - - edited oh oooooooh!!!!! 8-O cd /Applications/MAMP/htdocs/moodle19 grep -rn '<br/>' * 201 rows of output!!!!!! ( The occurrences number 111 is the one I rise up before!!!! He he, coherence preserved!!! )
        Hide
        Séverin Terrier added a comment -

        Yes, it would be good to replace all <br/> in Moodle by <br />

        Show
        Séverin Terrier added a comment - Yes, it would be good to replace all <br/> in Moodle by <br />
        Séverin Terrier made changes -
        Field Original Value New Value
        Summary tiny tiny tiny syntax error <br/> should be replaced by <br />
        Component/s General [ 10049 ]
        Component/s Database Activity [ 10092 ]
        Affects Version/s 1.9.3 [ 10290 ]
        Affects Version/s 1.9.2 [ 10280 ]
        Hide
        Eloy Lafuente (stronk7) added a comment - - edited

        Wow, they are really a lot! Agree we must change all them to "better" <br />.

        But, when saying "better" I'm not thinking about XHTML (because I think <br/> is perfectly valid) but because of compatibility with older browsers that could not recognize the "br/" tag and omit them, while "br /" will be rendered ok, becuase just the slash is a not recognized attribure and one simple "br" will be rendered.

        But, breaking XHTML? Uhm. I don't think it's broken because of that (perhaps I'm wrong). In other words: "XHTML 1.0 Recommendation suggests that all empty tags include a space before the slash"

        Ciao

        Show
        Eloy Lafuente (stronk7) added a comment - - edited Wow, they are really a lot! Agree we must change all them to "better" <br />. But, when saying "better" I'm not thinking about XHTML (because I think <br/> is perfectly valid) but because of compatibility with older browsers that could not recognize the "br/" tag and omit them, while "br /" will be rendered ok, becuase just the slash is a not recognized attribure and one simple "br" will be rendered. But, breaking XHTML? Uhm. I don't think it's broken because of that (perhaps I'm wrong). In other words: "XHTML 1.0 Recommendation suggests that all empty tags include a space before the slash" Ciao
        Hide
        Daniele Cordella added a comment -

        There is always something more to know.
        You are right, Eloy.
        I just asked to
        http://validator.w3.org/#validate_by_input
        to validate the following xhtml

        <!DOCTYPE html PUBLIC "-//W3C//DTD XHTML 1.0 Strict//EN"
        "http://www.w3.org/TR/xhtml1/DTD/xhtml1-strict.dtd">
        <html xmlns="http://www.w3.org/1999/xhtml">
        <head>
        <meta http-equiv="content-type" content="text/html; charset=utf-8" />
        <title>Untitled</title>
        </head>
        <body>
        <p>Hello<br/>
        world</p>
        </body>
        </html>
        and the answer was: "Congratulations
        The uploaded document was successfully checked as XHTML 1.0 Strict."

        Thanks, I didn't know.

        Show
        Daniele Cordella added a comment - There is always something more to know. You are right, Eloy. I just asked to http://validator.w3.org/#validate_by_input to validate the following xhtml <!DOCTYPE html PUBLIC "-//W3C//DTD XHTML 1.0 Strict//EN" "http://www.w3.org/TR/xhtml1/DTD/xhtml1-strict.dtd"> <html xmlns="http://www.w3.org/1999/xhtml"> <head> <meta http-equiv="content-type" content="text/html; charset=utf-8" /> <title>Untitled</title> </head> <body> <p>Hello<br/> world</p> </body> </html> and the answer was: "Congratulations The uploaded document was successfully checked as XHTML 1.0 Strict." Thanks, I didn't know.
        Martin Dougiamas made changes -
        Workflow jira [ 26692 ] MDL Workflow [ 43456 ]
        Martin Dougiamas made changes -
        Workflow MDL Workflow [ 43456 ] MDL Full Workflow [ 71849 ]
        Hide
        Michael de Raadt added a comment -

        Thanks for reporting this issue.

        We have detected that this issue has been inactive for over a year has been recorded as affecting versions that are no longer supported.

        If you believe that this issue is still relevant to current versions (2.1 and beyond), please comment on the issue. Issues left inactive for a further month will be closed.

        Michael d;

        lqjjLKA0p6

        Show
        Michael de Raadt added a comment - Thanks for reporting this issue. We have detected that this issue has been inactive for over a year has been recorded as affecting versions that are no longer supported. If you believe that this issue is still relevant to current versions (2.1 and beyond), please comment on the issue. Issues left inactive for a further month will be closed. Michael d; lqjjLKA0p6
        Hide
        Daniele Cordella added a comment -

        well... still 161 row of output comes by grepping current head.
        Some of them may be comments but, generally speaking, it is a pity to leave them unchanged!

        Show
        Daniele Cordella added a comment - well... still 161 row of output comes by grepping current head. Some of them may be comments but, generally speaking, it is a pity to leave them unchanged!
        Hide
        Anthony Borrow added a comment -

        I've gone ahead and marked this as affecting 2.x branches since Daniele confirms that there are still some things to be cleaned up. This may be a nice simple grep project for someone to get their feet wet. Peace - Anthony

        Show
        Anthony Borrow added a comment - I've gone ahead and marked this as affecting 2.x branches since Daniele confirms that there are still some things to be cleaned up. This may be a nice simple grep project for someone to get their feet wet. Peace - Anthony
        Anthony Borrow made changes -
        Affects Version/s 2.2 [ 10656 ]
        Affects Version/s 2.1 [ 10370 ]
        Affects Version/s 2.0.6 [ 11250 ]
        Hide
        Anthony Borrow added a comment -

        Eloy - Is this something that you would like to assign back to moodle.com? Peace - Anthony

        Show
        Anthony Borrow added a comment - Eloy - Is this something that you would like to assign back to moodle.com? Peace - Anthony
        Hide
        Eloy Lafuente (stronk7) added a comment -

        It's something I'd be happy to close as won't fix or not a bug, LOL!

        As stated above the only reasoning for the extra space were some old (really-really-old) browsers (all them not supported by Moodle 2.x at all).

        And, for XHTML... both are equally correct, so...

        ...as much, this could be considered one improvement (to be done for master only), but not backported to stables at all. Although I insist that my fav. option is to close this and ignore it.

        So, I'll leave this for MdR, in case he wants to add it to some moodle.com sprint or, alternatively, close it as won't fix.

        Ciao

        Show
        Eloy Lafuente (stronk7) added a comment - It's something I'd be happy to close as won't fix or not a bug, LOL! As stated above the only reasoning for the extra space were some old (really-really-old) browsers (all them not supported by Moodle 2.x at all). And, for XHTML... both are equally correct, so... ...as much, this could be considered one improvement (to be done for master only), but not backported to stables at all. Although I insist that my fav. option is to close this and ignore it. So, I'll leave this for MdR, in case he wants to add it to some moodle.com sprint or, alternatively, close it as won't fix. Ciao
        Hide
        Michael de Raadt added a comment -

        I'll take this into the next STABLE sprint.

        I've always been a fan of the space before the closing / but the main thing is that we are consistent.

        Show
        Michael de Raadt added a comment - I'll take this into the next STABLE sprint. I've always been a fan of the space before the closing / but the main thing is that we are consistent.
        Michael de Raadt made changes -
        Fix Version/s STABLE Sprint 17 [ 11550 ]
        Assignee Eloy Lafuente (stronk7) [ stronk7 ] moodle.com [ moodle.com ]
        Difficulty Easy [ 10023 ]
        Rajesh Taneja made changes -
        Assignee moodle.com [ moodle.com ] Rajesh Taneja [ rajeshtaneja ]
        Rajesh Taneja made changes -
        Status Open [ 1 ] Development in progress [ 3 ]
        Hide
        Rajesh Taneja added a comment -

        As this is just a cosmetic change, hence providing branch for master only.
        No changes made to third party lib(lib/tcpdf and lib/spikephpcoverage), so leaving some inconsistency there.

        Show
        Rajesh Taneja added a comment - As this is just a cosmetic change, hence providing branch for master only. No changes made to third party lib(lib/tcpdf and lib/spikephpcoverage), so leaving some inconsistency there.
        Rajesh Taneja made changes -
        Pull Master Diff URL https://github.com/rajeshtaneja/moodle/compare/master...wip-mdl-15040
        Pull Master Branch wip-mdl-15040
        Pull from Repository git://github.com/rajeshtaneja/moodle.git
        Hide
        Rajesh Taneja added a comment -

        Not sure if changes should go in moodle/backup/cc/schemas11/ccv1p1_imscp_v1p2_v1p0.xsd, and moodle/lib/tcpdf/tcpdf.php - Line 20657.
        Please advice.

        Show
        Rajesh Taneja added a comment - Not sure if changes should go in moodle/backup/cc/schemas11/ccv1p1_imscp_v1p2_v1p0.xsd , and moodle/lib/tcpdf/tcpdf.php - Line 20657. Please advice.
        Rajesh Taneja made changes -
        Status Development in progress [ 3 ] Waiting for peer review [ 10012 ]
        Rajesh Taneja made changes -
        Original Estimate 0 minutes [ 0 ]
        Remaining Estimate 0 minutes [ 0 ]
        Status Waiting for peer review [ 10012 ] Peer review in progress [ 10013 ]
        Peer reviewer abgreeve
        Hide
        Adrian Greeve added a comment -

        I had a look at the patch and the files that were not included for alteration.

        All of the moodle files have been converted. The only things left are third party programs.

        • Yui - javascript files
        • dragmath
        • spikephpcoverage
        • tcpdf
        • Common cartridge (I'm not sure if this is third party or not)

        Rajesh already mentioned that he wasn't altering third party files.

        Looks good.

        Thanks Raj.

        Show
        Adrian Greeve added a comment - I had a look at the patch and the files that were not included for alteration. All of the moodle files have been converted. The only things left are third party programs. Yui - javascript files dragmath spikephpcoverage tcpdf Common cartridge (I'm not sure if this is third party or not) Rajesh already mentioned that he wasn't altering third party files. Looks good. Thanks Raj.
        Adrian Greeve made changes -
        Status Peer review in progress [ 10013 ] Development in progress [ 3 ]
        Hide
        Rajesh Taneja added a comment -

        Thanks for the review Adrian
        Pushing it for integration review, if integrator feels that some of the left over files need change, then I will change it.

        Show
        Rajesh Taneja added a comment - Thanks for the review Adrian Pushing it for integration review, if integrator feels that some of the left over files need change, then I will change it.
        Rajesh Taneja made changes -
        Status Development in progress [ 3 ] Waiting for integration review [ 10010 ]
        Hide
        Eloy Lafuente (stronk7) added a comment -

        The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week.

        TIA and ciao

        Show
        Eloy Lafuente (stronk7) added a comment - The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week. TIA and ciao
        Hide
        Eloy Lafuente (stronk7) added a comment -

        Well,

        this is exactly the sort of change (whitespace) in master that will cause a lot of conflicts if the same parts of code are changed by other issues along the complete TTL of 21 and 22 stable (near 1 year).

        So IMO we should try to address this is another way. Possible alternatives:

        1) Backport the same changes to 21 and 22 STABLE too. It will cause problems ONCE to 3rd part developers having customizations. Exactly the same case that the one causing phpdocs not to be backported.

        2) Do it progressively. Each time some code is going to be modified and includes one tag of those, modify it to the "ruled" one (there is NO rule about that right now at all).

        3) Keep it as is. Both are legal 100%. Ruling about that is nosense.

        My personal opinion goes to 3). As much I would introduce one "warning" in the code checker, but I think it is nosense ruling about that.

        Ciao

        Show
        Eloy Lafuente (stronk7) added a comment - Well, this is exactly the sort of change (whitespace) in master that will cause a lot of conflicts if the same parts of code are changed by other issues along the complete TTL of 21 and 22 stable (near 1 year). So IMO we should try to address this is another way. Possible alternatives: 1) Backport the same changes to 21 and 22 STABLE too. It will cause problems ONCE to 3rd part developers having customizations. Exactly the same case that the one causing phpdocs not to be backported. 2) Do it progressively. Each time some code is going to be modified and includes one tag of those, modify it to the "ruled" one (there is NO rule about that right now at all). 3) Keep it as is. Both are legal 100%. Ruling about that is nosense. My personal opinion goes to 3). As much I would introduce one "warning" in the code checker, but I think it is nosense ruling about that. Ciao
        Eloy Lafuente (stronk7) made changes -
        Currently in integration Yes [ 10041 ]
        Hide
        Rajesh Taneja added a comment -

        They both are same and it's fine with me, if you don't integrate this patch
        But if you plan to integrate, then not sure why you want it progressively, as it's just for master.

        Please advice further action on this.

        Show
        Rajesh Taneja added a comment - They both are same and it's fine with me, if you don't integrate this patch But if you plan to integrate, then not sure why you want it progressively, as it's just for master. Please advice further action on this.
        Rajesh Taneja made changes -
        Testing Instructions No test required as it's just a cosmetic change.
        Will be nice to go though changes and see if you can find something wrong.
        Hide
        Daniele Cordella added a comment -

        You are Rajesh.
        I filed this bug by mistake.
        We are safe to close it.

        Show
        Daniele Cordella added a comment - You are Rajesh. I filed this bug by mistake. We are safe to close it.
        Hide
        Michael de Raadt added a comment -

        This seems like a non-issue now.

        It would be nice to have consistency, but if it comes at the cost of a large number of conflicts, then it's probably not worth implementing.

        Raj has created a solution that people can apply if they feel strongly about this.

        I'm happy for this issue to be closed.

        Show
        Michael de Raadt added a comment - This seems like a non-issue now. It would be nice to have consistency, but if it comes at the cost of a large number of conflicts, then it's probably not worth implementing. Raj has created a solution that people can apply if they feel strongly about this. I'm happy for this issue to be closed.
        Rajesh Taneja made changes -
        Attachment mdl-15040.patch [ 26699 ]
        Hide
        Rajesh Taneja added a comment - - edited

        Thanks Everyone,

        I am closing this issue as Not a bug, as everyone seems to be happy with current code

        You can use attached patch or run following on moodle code

        find ./ -type f -name "*.php" -exec sed -i 's/<br\/>/<br \/>/g' {} \;
        
        Show
        Rajesh Taneja added a comment - - edited Thanks Everyone, I am closing this issue as Not a bug , as everyone seems to be happy with current code You can use attached patch or run following on moodle code find ./ -type f -name "*.php" -exec sed -i 's/<br\/>/<br \/>/g' {} \;
        Rajesh Taneja made changes -
        Status Waiting for integration review [ 10010 ] Closed [ 6 ]
        Resolution Not a bug [ 7 ]
        Hide
        Eloy Lafuente (stronk7) added a comment -

        Thanks, guys!

        Show
        Eloy Lafuente (stronk7) added a comment - Thanks, guys!
        Eloy Lafuente (stronk7) made changes -
        Currently in integration Yes [ 10041 ]
        Eloy Lafuente (stronk7) made changes -
        Fix Version/s STABLE Sprint 17 [ 11550 ]

          People

          • Votes:
            2 Vote for this issue
            Watchers:
            2 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved: