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

      Description

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

        Gliffy Diagrams

          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: