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

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

    Details

    • Type: Bug
    • Status: Closed
    • Priority: 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

          Hide
          daniss 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
          daniss 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
          fox Séverin Terrier added a comment -

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

          Show
          fox Séverin Terrier added a comment - Yes, it would be good to replace all <br/> in Moodle by <br />
          Hide
          stronk7 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
          stronk7 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
          daniss 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
          daniss 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.
          Hide
          salvetore 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
          salvetore 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
          daniss 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
          daniss 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
          aborrow 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
          aborrow 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
          Hide
          aborrow Anthony Borrow added a comment -

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

          Show
          aborrow Anthony Borrow added a comment - Eloy - Is this something that you would like to assign back to moodle.com? Peace - Anthony
          Hide
          stronk7 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
          stronk7 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
          salvetore 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
          salvetore 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.
          Hide
          rajeshtaneja 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
          rajeshtaneja 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.
          Hide
          rajeshtaneja 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
          rajeshtaneja 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.
          Hide
          abgreeve 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
          abgreeve 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.
          Hide
          rajeshtaneja 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
          rajeshtaneja 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.
          Hide
          stronk7 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
          stronk7 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
          stronk7 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
          stronk7 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
          Hide
          rajeshtaneja 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
          rajeshtaneja 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.
          Hide
          daniss Daniele Cordella added a comment -

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

          Show
          daniss Daniele Cordella added a comment - You are Rajesh. I filed this bug by mistake. We are safe to close it.
          Hide
          salvetore 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
          salvetore 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.
          Hide
          rajeshtaneja 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
          rajeshtaneja 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' {} \;
          Hide
          stronk7 Eloy Lafuente (stronk7) added a comment -

          Thanks, guys!

          Show
          stronk7 Eloy Lafuente (stronk7) added a comment - Thanks, guys!

            People

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

              Dates

              • Created:
                Updated:
                Resolved: