Moodle
  1. Moodle
  2. MDL-38133

html_writer: add shortcuts for very frequent tags

    Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 2.4.1
    • Fix Version/s: 2.5
    • Component/s: Libraries
    • Rank:
      47955

      Description

      The html_writer class is generally convenient and safe but can result in rather long code. For example, if you want to add some accesshide text:

      echo html_writer::tag('span', $str, array('class' => 'accesside'));
      

      The problem is exacerbated when you are creating a structure of multiple tags.

      I propose creating shortcut methods for div and span, which are the most common structural tags. The shortcuts should also make it easier to add a CSS class as this is usually required for structural tags. This will allow for shorter code. (Other tags can continue to use the current methods.)

      Proposed code:

      echo html_writer::span($str, 'accesside');
      

        Issue Links

          Activity

          Hide
          Sam Marshall added a comment -

          Requesting peer review.

          Note: I submitted this because when I whined about it being a pain to write out <div> tags in the developer chat, Petr said, why don't you submit a patch... so I am!

          Show
          Sam Marshall added a comment - Requesting peer review. Note: I submitted this because when I whined about it being a pain to write out <div> tags in the developer chat, Petr said, why don't you submit a patch... so I am!
          Hide
          Sam Marshall added a comment -

          Note: Just to demonstrate that they are indeed commonly used: in the complete Moodle core code, as of now, there are 1,040 uses of html_writer for div tags and 246 for span tags, all of which could be shortened using these new functions.

          Regex used for search (example for span tag):

          html_writer::(start_|end_)?tag\('span'
          
          Show
          Sam Marshall added a comment - Note: Just to demonstrate that they are indeed commonly used: in the complete Moodle core code, as of now, there are 1,040 uses of html_writer for div tags and 246 for span tags, all of which could be shortened using these new functions. Regex used for search (example for span tag): html_writer::(start_|end_)?tag\('span'
          Hide
          David Mudrak added a comment -

          Very nice. Thanks a lot Sam.

          [Y] Syntax
          [Y] Output
          [Y] Whitespace
          [-] Language
          [-] Databases
          [-] Testing (haven't run unit tests but they are present and good quality)
          [-] Security
          [-] Documentation (please add dev_docs_required and api_change labels)
          [Y] Git
          [Y] Sanity check

          Show
          David Mudrak added a comment - Very nice. Thanks a lot Sam. [Y] Syntax [Y] Output [Y] Whitespace [-] Language [-] Databases [-] Testing (haven't run unit tests but they are present and good quality) [-] Security [-] Documentation (please add dev_docs_required and api_change labels) [Y] Git [Y] Sanity check
          Hide
          Michael de Raadt added a comment -

          I'm neither here nor there on this one. It will shorten code a bit, but it will add to the complexity of the code.

          Show
          Michael de Raadt added a comment - I'm neither here nor there on this one. It will shorten code a bit, but it will add to the complexity of the code.
          Hide
          David Mudrak added a comment -

          I don't think that shortening the code is the primary target here. The real problem is that if you try to be a good boy using output renderers, html_writer and CSS classes (as you should), producing a single <div> just sucks (and as theme designers demand, you should produce them a lot). This patch makes it suck less.

          Show
          David Mudrak added a comment - I don't think that shortening the code is the primary target here. The real problem is that if you try to be a good boy using output renderers, html_writer and CSS classes (as you should), producing a single <div> just sucks (and as theme designers demand, you should produce them a lot). This patch makes it suck less.
          Hide
          Sam Marshall added a comment -

          Thanks for review David. Submitting for integration, let's see what they think.

          Btw, my opinion would be that this makes code simpler, not more complex.

          Show
          Sam Marshall added a comment - Thanks for review David. Submitting for integration, let's see what they think. Btw, my opinion would be that this makes code simpler, not more complex.
          Hide
          Damyon Wiese 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.

          Thanks!

          Show
          Damyon Wiese 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. Thanks!
          Hide
          Tim Hunt added a comment -

          These mostly look good, but I wonder if include_tag should also handle the case where $attributes already includes 'class'. It could add the new class name to that with a space, rather than overwriting it. Also, the function might better be called add_class.

          Show
          Tim Hunt added a comment - These mostly look good, but I wonder if include_tag should also handle the case where $attributes already includes 'class'. It could add the new class name to that with a space, rather than overwriting it. Also, the function might better be called add_class.
          Hide
          Damyon Wiese added a comment -

          Actually - I dislike this change.

          The proper way to write a div is with a renderer->container method so it can be overridden with a theme. There are few exceptions mostly in old code - or renderers themselves.

          Adding this to html_writer implies that we want people to use html_writer::div everywhere.

          Show
          Damyon Wiese added a comment - Actually - I dislike this change. The proper way to write a div is with a renderer->container method so it can be overridden with a theme. There are few exceptions mostly in old code - or renderers themselves. Adding this to html_writer implies that we want people to use html_writer::div everywhere.
          Hide
          Dan Poltawski added a comment -

          I don't really agree with Damyon, especially as its really really common for a div to be used for something much smaller than a proper container method would imply. We already have thousands of them.

          But, I think Tim's suggestion is a good one. (or at least warning the developer that their attribute is being ignored). I also think it would be worth starting a forum discussion about this, as there clearly are differing views.

          Show
          Dan Poltawski added a comment - I don't really agree with Damyon, especially as its really really common for a div to be used for something much smaller than a proper container method would imply. We already have thousands of them. But, I think Tim's suggestion is a good one. (or at least warning the developer that their attribute is being ignored). I also think it would be worth starting a forum discussion about this, as there clearly are differing views.
          Hide
          CiBoT added a comment -

          Moving this reopened issue out from current integration. Please, re-submit it for integration once ready.

          Show
          CiBoT added a comment - Moving this reopened issue out from current integration. Please, re-submit it for integration once ready.
          Hide
          David Mudrak added a comment -

          I must slightly disagree with Damyon. I see the div elements as something that renderers use to build bigger parts of the page - such as a post forum post, submission, continue button etc. Themes may want to override these bigger blocks, change their order, inject something between them etc. But I have never seen a reasonable use case for actually replacing <div> produced by the container() with something else. Using the container() method has some advantages. But that does not make html_writer::tag('div') obsolete in any way. Note that when container() was implemented, we did not have the html_writer yet.

          And yes, Tim raised good point, as Dan noted.

          Show
          David Mudrak added a comment - I must slightly disagree with Damyon. I see the div elements as something that renderers use to build bigger parts of the page - such as a post forum post, submission, continue button etc. Themes may want to override these bigger blocks, change their order, inject something between them etc. But I have never seen a reasonable use case for actually replacing <div> produced by the container() with something else. Using the container() method has some advantages. But that does not make html_writer::tag('div') obsolete in any way. Note that when container() was implemented, we did not have the html_writer yet. And yes, Tim raised good point, as Dan noted.
          Hide
          Sam Marshall added a comment -

          Dan: I've made a forum post for this

          https://moodle.org/mod/forum/discuss.php?d=223092

          I included in that post some of the arguments from this issue while simultaneously shooting them down, which is a bit unfair, but let's see how it goes.

          Tim/etc: I've changed code as suggested. (Waiting on phpunit init so I can actually run the new tests, will push again if I broke it.)

          Show
          Sam Marshall added a comment - Dan: I've made a forum post for this https://moodle.org/mod/forum/discuss.php?d=223092 I included in that post some of the arguments from this issue while simultaneously shooting them down, which is a bit unfair, but let's see how it goes. Tim/etc: I've changed code as suggested. (Waiting on phpunit init so I can actually run the new tests, will push again if I broke it.)
          Hide
          Sam Marshall added a comment -

          (I did break it, but have pushed working version now.)

          Show
          Sam Marshall added a comment - (I did break it, but have pushed working version now.)
          Hide
          Damyon Wiese added a comment -

          OK - I have been convinced that there are enough fair uses of this in the renderers to make this worthwhile.

          Show
          Damyon Wiese added a comment - OK - I have been convinced that there are enough fair uses of this in the renderers to make this worthwhile.
          Hide
          Sam Marshall added a comment -

          Thanks Damyon, all. Resubmitting for integration next time - I got a couple more positive reponses in the forum discussion, nobody was against.

          Show
          Sam Marshall added a comment - Thanks Damyon, all. Resubmitting for integration next time - I got a couple more positive reponses in the forum discussion, nobody was against.
          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 -

          Integrated (master only), thanks!

          Note this requires dev documentation, api changes, don't forget it.

          Ciao

          Show
          Eloy Lafuente (stronk7) added a comment - Integrated (master only), thanks! Note this requires dev documentation, api changes, don't forget it. Ciao
          Hide
          Sam Marshall added a comment -

          I wrote dev documentation

          http://docs.moodle.org/dev/html_writer#div.2C_span_methods

          Not sure if this needs linking from somewhere

          Show
          Sam Marshall added a comment - I wrote dev documentation http://docs.moodle.org/dev/html_writer#div.2C_span_methods Not sure if this needs linking from somewhere
          Hide
          David Monllaó added a comment -

          Passing as running in the CI server

          Show
          David Monllaó added a comment - Passing as running in the CI server
          Hide
          Eloy Lafuente (stronk7) added a comment -

          This is valid for unlimited entries to the, soon to be unveiled, Moodle Codebase Gardens. It includes free access to all facilities.

          Personal and non-transferable to all assignees, reviewers and testers in this issue. Valid until switching to Blackboard (100000€ penalization will be applied).

          Thanks, closing as fixed!

          Show
          Eloy Lafuente (stronk7) added a comment - This is valid for unlimited entries to the, soon to be unveiled, Moodle Codebase Gardens. It includes free access to all facilities. Personal and non-transferable to all assignees, reviewers and testers in this issue. Valid until switching to Blackboard (100000€ penalization will be applied). Thanks, closing as fixed!
          Hide
          David Scotson added a comment -

          Added a link to a couple of similar changes.

          Show
          David Scotson added a comment - Added a link to a couple of similar changes.

            People

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

              Dates

              • Created:
                Updated:
                Resolved: