Details

    • Type: Sub-task Sub-task
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 2.0
    • Fix Version/s: 2.0
    • Component/s: Themes
    • Labels:
      None
    • Affected Branches:
      MOODLE_20_STABLE
    • Fixed Branches:
      MOODLE_20_STABLE
    • Rank:
      32858

      Description

      1/ the old way - $OUTPUT->single_button($url, $label, ...) which allows single_button object in first param too

      benefits:

      • extremely easy migration from print_single_button() in 1.9
      • easy for new ppl that use code completion in IDEs - type $OUUTPUT ==> then you get list of methods ==> then you get all single_button() parameters and links to advanced usage
      • make methods are optionally possible too

      2/ make factory methods

      $OUTPUT->print(single_button::make($url, $label, ...))

      problems:

      • no code completion possible
      • totally different from 1.9
      • custom OOP style (not in Java or anything else I know)
      • Eloy does not like static methods :-P

      3/ $OUTPUT->button(html_form::make_single_button())
      it has the worst of both worlds

      Please submit more cons and pros as comments

        Issue Links

          Activity

          Hide
          Petr Škoda added a comment -

          hmm, $OUTPUT->output(something::make())? it does not actually print, just returns string

          Show
          Petr Škoda added a comment - hmm, $OUTPUT->output(something::make())? it does not actually print, just returns string
          Hide
          David Mudrak added a comment -

          I am fine with the first one 1/ if we are consistent with the ability to pass the component as the first param. I know Petr already started this way, like in

          link($link_or_url, $text = null, array $options = null)
          

          and I think it is a nice compromise between those preferring one-liners and those who prefer explicitness.

          The only comment is that $link_or_url violates our coding guidelines as we do not use underscores in variables. I can see the benefit of the exception here, though, so I will be OK with it.

          Our documentation then should provide both possible usages:

          // explicit style
          $link = new html_link();
          $link->set_properties();
          echo $OUTPUT->link($link);
          
          // one-line style
          echo $OUTPUT->link('http://moodle.org', array('property' => 'value'));
          
          Show
          David Mudrak added a comment - I am fine with the first one 1/ if we are consistent with the ability to pass the component as the first param. I know Petr already started this way, like in link($link_or_url, $text = null , array $options = null ) and I think it is a nice compromise between those preferring one-liners and those who prefer explicitness. The only comment is that $link_or_url violates our coding guidelines as we do not use underscores in variables. I can see the benefit of the exception here, though, so I will be OK with it. Our documentation then should provide both possible usages: // explicit style $link = new html_link(); $link->set_properties(); echo $OUTPUT->link($link); // one-line style echo $OUTPUT->link('http: //moodle.org', array('property' => 'value'));
          Hide
          Martin Dougiamas added a comment -

          Keep talking here, we'll meet about this tomorrow (Wednesday 6th at 4:00pm Perth time, 9am Europe and 9pm Wellington)

          Show
          Martin Dougiamas added a comment - Keep talking here, we'll meet about this tomorrow (Wednesday 6th at 4:00pm Perth time, 9am Europe and 9pm Wellington)
          Hide
          Sam Hemelryk added a comment -

          My 2 cents:

          As far as I can see all three options introduce the same complexities,
          Method one we'll end up with a large number of methods against renderers, this reintroduces the mass of methods weblib used to have, one of the things the output was designed to combat.
          In saying that we need to have variety somewhere, my preference is towards option 3 ... because ...

          • I like type hinting I think it is just a great as autocompletion but at a code level, I would prefer to see flexibilty achieved through various methods against the desired object (in this case a component)
          • It results in less clutter against the core renderer

          ..... hopefully that makes a little sense, at the end of the day I'll be happy with which ever way we go

          The only thing I like about method 2 is that we wouldn't need to match apples to apples and buttons to buttons.

          On a side note I think we should also try to keep the number of components to a minimum...

          Show
          Sam Hemelryk added a comment - My 2 cents: As far as I can see all three options introduce the same complexities, Method one we'll end up with a large number of methods against renderers, this reintroduces the mass of methods weblib used to have, one of the things the output was designed to combat. In saying that we need to have variety somewhere, my preference is towards option 3 ... because ... I like type hinting I think it is just a great as autocompletion but at a code level, I would prefer to see flexibilty achieved through various methods against the desired object (in this case a component) It results in less clutter against the core renderer ..... hopefully that makes a little sense, at the end of the day I'll be happy with which ever way we go The only thing I like about method 2 is that we wouldn't need to match apples to apples and buttons to buttons. On a side note I think we should also try to keep the number of components to a minimum...
          Hide
          David Mudrak added a comment -

          Sam, can you elaborate on introducing the mess as the weblib used to have? I can see that 1/ is just convenient shortcut to 3/. Let the code express what I mean:

          /** this is hypothetical $output->submission() */
          public function submission($submission_or_title, $content = null) {
              if ($submission_or_title instanceof submission_component) {
                  $submission = $submission_or_title; // do we have to use clone() here?
                  if (func_num_args() > 1) {
                      debugging('All other params are ignored');
                  }
              } elseif (is_string($submission_or_title)) {
                  $submission = new submission_component($submission_or_title, $content);
              } else {
                  throw new coding_exception('Must pass either submission component or title+content');
              }
              // and now we can render the submission, this would go to a protected method, probably
              return $this->render_submission($submission);
          }
          
          Show
          David Mudrak added a comment - Sam, can you elaborate on introducing the mess as the weblib used to have? I can see that 1/ is just convenient shortcut to 3/. Let the code express what I mean: /** this is hypothetical $output->submission() */ public function submission($submission_or_title, $content = null ) { if ($submission_or_title instanceof submission_component) { $submission = $submission_or_title; // do we have to use clone() here? if (func_num_args() > 1) { debugging('All other params are ignored'); } } elseif (is_string($submission_or_title)) { $submission = new submission_component($submission_or_title, $content); } else { throw new coding_exception('Must pass either submission component or title+content'); } // and now we can render the submission, this would go to a protected method, probably return $ this ->render_submission($submission); }
          Hide
          Petr Škoda added a comment -

          " I would prefer to see flexibilty achieved through various methods against the desired object (in this case a component)" - the problem here is that we can not abstract things with OOP design based on html_components. Take for example user picture - it may be either image or image wrapped in link. The nature of out structures does not replicate the DOM model which is what the html_component is trying to abstract in PHP.

          Half the components do not have any relation to the html_component stuff, for example:

          • standard_end_of_body_html()
          • update_module_button()
          • fatal_error()
          • htmlattributes()
          • rarrow()
          • has_started()

          I am not agains simple DOM emulation framework which help tremendously with XSS problems, but I do nto think the core_renderer is a place to define it because:
          1/ no theme is supposed to override these low level things like printing of <a> or <img> tags
          2/ use of these really low level methods goes directly against the theme customisation of via renderers overriding - the plugin code should use as few renderer parameters as possible and let the themes decide!!

          Ideally renderers should contain really "heavy" methods that do rendering of large chunks of UI code so that theme designers may decide to reinvent these things.

          I would personally vote to move out all the html_component stuff from core_renderer and better not use these at all in normal code (understand modules) because using of html_component and children in module code goes directly against easy UI customisations via themes.

          I think we should try to imagine the renderers as magic functions that create templates on the fly - we do not actually care how it works inside, or how the data gets printed.

          From the API point of view following is important:
          1/ to have bigger methods in renderers
          2/ to have as many renderers as possible (remember the more methods the easier UI customisations)
          3/ to pass enough information to each renderer

          If you need example of really useful renderer methods please have a look at login_info(), lang_menu(), doc_link(). These can be easily overridden in themes, they significantly simplify coding because you do not have to copy paste the code over and over again.

          Some bad examples are checkbox(), field() - why the hack should we construct such low level things in our code? We have formslib for that (or it least it was supposed to do that). These should definitely not be part of public core_render API. the should live in separate library for rendering of general DOM which would be used from the "real" renderer methods.

          Another ugly example is the use of html_form in gradebook reports, that whole custom table enclosed in html_form should be in renderer, this way it can not be altered via themes at all (except the standard CSS), this completely defeats the purpose of renderers IMO.

          hmmmmmm

          Show
          Petr Škoda added a comment - " I would prefer to see flexibilty achieved through various methods against the desired object (in this case a component)" - the problem here is that we can not abstract things with OOP design based on html_components. Take for example user picture - it may be either image or image wrapped in link. The nature of out structures does not replicate the DOM model which is what the html_component is trying to abstract in PHP. Half the components do not have any relation to the html_component stuff, for example: standard_end_of_body_html() update_module_button() fatal_error() htmlattributes() rarrow() has_started() I am not agains simple DOM emulation framework which help tremendously with XSS problems, but I do nto think the core_renderer is a place to define it because: 1/ no theme is supposed to override these low level things like printing of <a> or <img> tags 2/ use of these really low level methods goes directly against the theme customisation of via renderers overriding - the plugin code should use as few renderer parameters as possible and let the themes decide!! Ideally renderers should contain really "heavy" methods that do rendering of large chunks of UI code so that theme designers may decide to reinvent these things. I would personally vote to move out all the html_component stuff from core_renderer and better not use these at all in normal code (understand modules) because using of html_component and children in module code goes directly against easy UI customisations via themes. I think we should try to imagine the renderers as magic functions that create templates on the fly - we do not actually care how it works inside, or how the data gets printed. From the API point of view following is important: 1/ to have bigger methods in renderers 2/ to have as many renderers as possible (remember the more methods the easier UI customisations) 3/ to pass enough information to each renderer If you need example of really useful renderer methods please have a look at login_info(), lang_menu(), doc_link(). These can be easily overridden in themes, they significantly simplify coding because you do not have to copy paste the code over and over again. Some bad examples are checkbox(), field() - why the hack should we construct such low level things in our code? We have formslib for that (or it least it was supposed to do that). These should definitely not be part of public core_render API. the should live in separate library for rendering of general DOM which would be used from the "real" renderer methods. Another ugly example is the use of html_form in gradebook reports, that whole custom table enclosed in html_form should be in renderer, this way it can not be altered via themes at all (except the standard CSS), this completely defeats the purpose of renderers IMO. hmmmmmm
          Hide
          Petr Škoda added a comment -

          oh, forgot to mention that the large number of core renderers can be handled by adding core renderer subtyes - such as renderers for group UI, separate renderer for roles UI, etc.

          Show
          Petr Škoda added a comment - oh, forgot to mention that the large number of core renderers can be handled by adding core renderer subtyes - such as renderers for group UI, separate renderer for roles UI, etc.
          Hide
          Petr Škoda added a comment -

          oh, these complex renderer methods in these core renderer subtypes will definitely not accept anything derived from html_component, they accept just some general information not related to HTML DOM and spit out chunk of HTML.

          Show
          Petr Škoda added a comment - oh, these complex renderer methods in these core renderer subtypes will definitely not accept anything derived from html_component, they accept just some general information not related to HTML DOM and spit out chunk of HTML.
          Hide
          David Mudrak added a comment -

          Petr, why not to have an "avatar" (or user_picture) component that is basically a stdclass with properties like userid and link? If link is null, no <a> wrapper is rendered. The user_picture->prepare() method would just find if the user has uploaded his/her photo or used the default one and would add a protected property $imgsrc or whatever so the renderer just uses it to actually display the image.

          I am asking for this because I do not get your note in "the problem here is that we can not abstract things..."

          Show
          David Mudrak added a comment - Petr, why not to have an "avatar" (or user_picture) component that is basically a stdclass with properties like userid and link? If link is null, no <a> wrapper is rendered. The user_picture->prepare() method would just find if the user has uploaded his/her photo or used the default one and would add a protected property $imgsrc or whatever so the renderer just uses it to actually display the image. I am asking for this because I do not get your note in "the problem here is that we can not abstract things..."
          Hide
          Petr Škoda added a comment -

          renderer is not not html rendering framework, the idea is to take general parameters and produce some HTML fragment, we should not dictate use checkbox for this, use html select for that use <ul> or any other DOM element, this decision should be made by the renderer method itself.

          Back to the user avatar - it does not matter much how we describe the user and how we give other extra hints, but to me it seems wrong to use HTML DOM abstraction for that because themes may decide to use ANY html markup to actually do that. If we use anything derived from current html_component we add a lot more information than we might expect to the renderer and then any custom theme that decided to render avatars in a different way - such as using "gravatars" for example is screwed. In fact I wanted to pass more user details in - maybe even complete user record:
          1/ gravatars need email in order to make hack from it
          2/ we might render mnet images with some funny overlay - so mnethostid could be handy too
          3/ somebody may decide to add different colour background to avatars based on some other criteria such as user participation

          Anything should be possible with the new renderers, unfortunately by using html_component we are making the public API unnecessarily complex for any unexpected customisation. If you do not like how core_renderer::user_picture() works fine, make a new renderer in your plugin and customise it, that is the expected way - not adding tons of useless features and html_component dependencies in core_renderer.

          Also for accessibility reasons the avatars should behave in a predictable way - for example link to user course profile or nothing at all. If it links to different place on each page it would be a usability nightmare, it should always open new window or never, etc. That is the reason why we should not encourage customisable link parameter in avatars, if somebody really really needs that they may use normal avatar without link and wrap it. I personally this that the only reasonable extra parameters are: print link and class for image which actually sets the image size.

          I now fully understand the desperation of theme designers when developers start inventing "clever" & complex OOP frameworks for printing of html markup

          Show
          Petr Škoda added a comment - renderer is not not html rendering framework, the idea is to take general parameters and produce some HTML fragment, we should not dictate use checkbox for this, use html select for that use <ul> or any other DOM element, this decision should be made by the renderer method itself. Back to the user avatar - it does not matter much how we describe the user and how we give other extra hints, but to me it seems wrong to use HTML DOM abstraction for that because themes may decide to use ANY html markup to actually do that. If we use anything derived from current html_component we add a lot more information than we might expect to the renderer and then any custom theme that decided to render avatars in a different way - such as using "gravatars" for example is screwed. In fact I wanted to pass more user details in - maybe even complete user record: 1/ gravatars need email in order to make hack from it 2/ we might render mnet images with some funny overlay - so mnethostid could be handy too 3/ somebody may decide to add different colour background to avatars based on some other criteria such as user participation Anything should be possible with the new renderers, unfortunately by using html_component we are making the public API unnecessarily complex for any unexpected customisation. If you do not like how core_renderer::user_picture() works fine, make a new renderer in your plugin and customise it, that is the expected way - not adding tons of useless features and html_component dependencies in core_renderer. Also for accessibility reasons the avatars should behave in a predictable way - for example link to user course profile or nothing at all. If it links to different place on each page it would be a usability nightmare, it should always open new window or never, etc. That is the reason why we should not encourage customisable link parameter in avatars, if somebody really really needs that they may use normal avatar without link and wrap it. I personally this that the only reasonable extra parameters are: print link and class for image which actually sets the image size. I now fully understand the desperation of theme designers when developers start inventing "clever" & complex OOP frameworks for printing of html markup
          Hide
          Petr Škoda added a comment -

          Here is a list of methods that imo do not belong into core_renderer and should be instead abstracted into separate DOM emulation layer:

          • field()
          • htmlattributes()
          • htmllist()
          • label()
          • select()
          • radio()
          • span()
          • textfield()
            these methods should not appear in mod/xxx/view.php or lib.php, those should be used only from renderer methods

          Instead they could be moved into separate HTML DOM library that consists of exact emulation of DOM html tree that may be constructed in PHP memory and then simply printed with __toString() call.

          As a replacement we would need to add new methods into core_renderer:

          • single_select() replaces old popup
          • single_textbox() for searching and similar

          The half cooked gradebook plugin conversion would be converted to use the new HTML DOM library and later would be properly converted to renderers.

          Options in existing core_renderer methods would be updated to support actions too, this was in fact the main reason for supporting single parameter that included actions.

          We could try to find some existing PHP DOM emulation library and only extend it a bit, or refactore current html_component and friends.

          It is unfortunately that we already did the mass conversion to new OUTPUT, on the other hand I already refactored a lot of the uses while creating specialised methods such as help_icon() or single_button().

          Technically we just need to finalise the public API now, and decide if we want the true emulated PHP DOM tree instead of the current lib/outputcomponents.php.

          Going to make some examples and sample code usage...

          Show
          Petr Škoda added a comment - Here is a list of methods that imo do not belong into core_renderer and should be instead abstracted into separate DOM emulation layer: field() htmlattributes() htmllist() label() select() radio() span() textfield() these methods should not appear in mod/xxx/view.php or lib.php, those should be used only from renderer methods Instead they could be moved into separate HTML DOM library that consists of exact emulation of DOM html tree that may be constructed in PHP memory and then simply printed with __toString() call. As a replacement we would need to add new methods into core_renderer: single_select() replaces old popup single_textbox() for searching and similar The half cooked gradebook plugin conversion would be converted to use the new HTML DOM library and later would be properly converted to renderers. Options in existing core_renderer methods would be updated to support actions too, this was in fact the main reason for supporting single parameter that included actions. We could try to find some existing PHP DOM emulation library and only extend it a bit, or refactore current html_component and friends. It is unfortunately that we already did the mass conversion to new OUTPUT, on the other hand I already refactored a lot of the uses while creating specialised methods such as help_icon() or single_button(). Technically we just need to finalise the public API now, and decide if we want the true emulated PHP DOM tree instead of the current lib/outputcomponents.php. Going to make some examples and sample code usage...
          Hide
          David Mudrak added a comment -

          Makes sense to me to remove those methods from core_renderer class. Moodle core_renderer should provide low-level bricks like heading(), container(), box() etc. I am afraid we will need to provide a way for module renderers to somehow generate whatever HTML they want, still. I mean, module renderer must be able to produce any HTML the developer needs (should be a rare case, though).

          I am not sure we need a special DOM generator library. On the other hand, that is an implementation detail and such a library should be used by the lowest level of Moodle renderers only.

          Show
          David Mudrak added a comment - Makes sense to me to remove those methods from core_renderer class. Moodle core_renderer should provide low-level bricks like heading(), container(), box() etc. I am afraid we will need to provide a way for module renderers to somehow generate whatever HTML they want, still. I mean, module renderer must be able to produce any HTML the developer needs (should be a rare case, though). I am not sure we need a special DOM generator library. On the other hand, that is an implementation detail and such a library should be used by the lowest level of Moodle renderers only.
          Hide
          Petr Škoda added a comment -

          David, you know to not use "these" low level html building methods from $OUTPUT in your view.php and lib.php, unfortunately other developers do not understand and they think it is perfect to hack complex html forms and advanced UI elements using the html_component stuff directly from view.php....

          The separation of DOM emulation is supposed to make it obvious that it is not a renderer because it is not available as $OUTPUT-.> methods, then everybody would see immediately that the renderers are not used properly or at all.

          This is not theoretical, we have many examples of incorrect use of $OUTPTU from the recent conversion, and remember those were core developers, imagine contrib devs copy pasting those problematic bits and using them all over their contrib mods instead of creating proper module renderer classes.

          I do not think it matters much what kind of api or abstraction we use - the target is that all developers create code that is easy to maintain and easy to customise in themes, developer friendliness is also very important - not everybody understands OOP, ppl tend to copy/paste a lot.

          I do seem many proofs in cvs that current design leads to sloppy coding and potential problems in future. I feel confident that my proposed changes will help prevent these problems and both new and old developers could benefit in the long term, theme designers would be positively affected too. Does anybody have arguments other than own personal coding preferences?

          Show
          Petr Škoda added a comment - David, you know to not use "these" low level html building methods from $OUTPUT in your view.php and lib.php, unfortunately other developers do not understand and they think it is perfect to hack complex html forms and advanced UI elements using the html_component stuff directly from view.php.... The separation of DOM emulation is supposed to make it obvious that it is not a renderer because it is not available as $OUTPUT-.> methods, then everybody would see immediately that the renderers are not used properly or at all. This is not theoretical, we have many examples of incorrect use of $OUTPTU from the recent conversion, and remember those were core developers, imagine contrib devs copy pasting those problematic bits and using them all over their contrib mods instead of creating proper module renderer classes. I do not think it matters much what kind of api or abstraction we use - the target is that all developers create code that is easy to maintain and easy to customise in themes, developer friendliness is also very important - not everybody understands OOP, ppl tend to copy/paste a lot. I do seem many proofs in cvs that current design leads to sloppy coding and potential problems in future. I feel confident that my proposed changes will help prevent these problems and both new and old developers could benefit in the long term, theme designers would be positively affected too. Does anybody have arguments other than own personal coding preferences?
          Hide
          Petr Škoda added a comment - - edited

          examples
          1/ original print_checkbox()
          http://cvs.moodle.org/moodle/admin/report/unittest/dbtest.php?r1=1.11&r2=1.8

          The checkbox can not be actually styled by any theme, any change would affect many other places. The checkbox does not have anything to do with current page or theme.
          There are several options how to deal with this part of code:
          a/ new renderer stored in admin/report/unittest/renderer.pgp which is a plugin renderer and implement method run_test_box($showpasses, $codecoverage, $dbinfos, $selected)
          b/ convert the thing to true moodle form using mforms
          c/ hardcode normal tags (the really old way)
          d/ use the proposed DOM emulation and construct the elements instances and simply convert to string which would just replace

          echo '<p>'; echo $OUTPUT->checkbox(html_select_option::make_checkbox(1, $showpasses, get_string('showpasses', 'simpletest')), 'showpasses') ; echo '</p>';

          with

          echo '<p>'; echo html_select_option::make_checkbox(1, $showpasses, get_string('showpasses', 'simpletest')) ; echo '</p>';

          or something similar to this

          Show
          Petr Škoda added a comment - - edited examples 1/ original print_checkbox() http://cvs.moodle.org/moodle/admin/report/unittest/dbtest.php?r1=1.11&r2=1.8 The checkbox can not be actually styled by any theme, any change would affect many other places. The checkbox does not have anything to do with current page or theme. There are several options how to deal with this part of code: a/ new renderer stored in admin/report/unittest/renderer.pgp which is a plugin renderer and implement method run_test_box($showpasses, $codecoverage, $dbinfos, $selected) b/ convert the thing to true moodle form using mforms c/ hardcode normal tags (the really old way) d/ use the proposed DOM emulation and construct the elements instances and simply convert to string which would just replace echo '<p>'; echo $OUTPUT->checkbox(html_select_option::make_checkbox(1, $showpasses, get_string('showpasses', 'simpletest')), 'showpasses') ; echo '</p>'; with echo '<p>'; echo html_select_option::make_checkbox(1, $showpasses, get_string('showpasses', 'simpletest')) ; echo '</p>'; or something similar to this
          Hide
          Tim Hunt added a comment -

          Seems like a good discussion that is heading in the right direction without me saying anything. However, I just want to make a few points:

          Remember that the main point is that in the future, core_renderer should not really be used much. Modules should have their own renderers that work on a much higher level, for example $forumoutput->discussion(...) that are much more useful for themers to override. (As David said.)

          So in a sense, most of what core_renderer is doing is making it easy to convert old code that used to use weblib print_... methods, as a first step in moving towards this future vision. That is, a lot of core_renderer may end up deprecated evenutally, and the key consideration for those parts is making it easy for people to update their code.

          The other job for core renderer is providing consistent rendering of certain components that the larger chunks like $forumoutput->discussion(...) need. $OUTPUT->user_picture is a good example of this sort of thing.

          My vote goes for this proposal of David's: http://tracker.moodle.org/browse/MDL-21235?focusedCommentId=79193&page=com.atlassian.jira.plugin.system.issuetabpanels%3Acomment-tabpanel#action_79193

          That is, make $OUTPUT->user_picture take either a user_picture object that lets you control all the options, or it takes a few common options as a short cut - but no more than about 4 common options. If you want to set really complex options, you need to create an object.

          That makes nice use of the flexibility that PHP gives us, while at the same time keeping the code easy to read and maintain.

          Show
          Tim Hunt added a comment - Seems like a good discussion that is heading in the right direction without me saying anything. However, I just want to make a few points: Remember that the main point is that in the future, core_renderer should not really be used much. Modules should have their own renderers that work on a much higher level, for example $forumoutput->discussion(...) that are much more useful for themers to override. (As David said.) So in a sense, most of what core_renderer is doing is making it easy to convert old code that used to use weblib print_... methods, as a first step in moving towards this future vision. That is, a lot of core_renderer may end up deprecated evenutally, and the key consideration for those parts is making it easy for people to update their code. The other job for core renderer is providing consistent rendering of certain components that the larger chunks like $forumoutput->discussion(...) need. $OUTPUT->user_picture is a good example of this sort of thing. My vote goes for this proposal of David's: http://tracker.moodle.org/browse/MDL-21235?focusedCommentId=79193&page=com.atlassian.jira.plugin.system.issuetabpanels%3Acomment-tabpanel#action_79193 That is, make $OUTPUT->user_picture take either a user_picture object that lets you control all the options, or it takes a few common options as a short cut - but no more than about 4 common options. If you want to set really complex options, you need to create an object. That makes nice use of the flexibility that PHP gives us, while at the same time keeping the code easy to read and maintain.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Side comment1: Why the hell are we being renderer-centric instead of component-centric? I always thought that each component (box, button, list, table, header...) was going to support different renderers (core_renderer, workshop_renderer, cli_renderer, pdf_renderer...) instead of having each renderer implementing/overriding some/all the components. Obviously it's too late to change this. Just wanted to side comment it, because it can have implications in my comments below.

          Side comment2: IMO we are also having an important overlapping between formslib and outputlib/componentslib and, in the long term... that needs to be solved in a clever way. Else it's, once more, duplicate functionality and, worse, none of them are 100% successful (with formslib missing important rendering possibilities and componentslib missing all the (cool) validation stuff and so on).

          Side comment3: I'd delete all those "html_" prefixes from the components. Just let them to be "buttons", "checkboxes", "select menus"... whatever, but without being html ones.

          About the current issue:

          One of the (potentially) good things of having everything under the $OUTPUT umbrella is that I can, at any moment, create my own complete pdf renderer, text renderer... whatever, then replace $OUTPUT and, voilà, I'll have the page rendered in one different format. I think this is the approach followed by the cli_renderer, useful to perform installations and so on from terminal. In other words, $OUTPUT (the class), has total control over everything (the components and how they are finally rendered). I know this (complete text/pdf generation) is a stupid case right now, just the idea of the renderer being the boss is the key bit.

          If we use alternative 2, we are, for sure, losing that point, so the components themselves will be in charge of "rendering" the output. And that's nono (remember? we are (grrr) renderer-centric, so only renderers must be able to generate html (or text or pdf) code).

          So we end with alternatives 1 and 3, both pretty similar, and it's about to decide if we want to pass a long_list_of_params (like current stable weblib) or one component object created via factory method, well, I'd say NONE of them (nor both, as commented by David and Tim).

          In any case, I'd replace any long_list_of_params by one array_of_params (that avoids being adding params continuously). And in any case, I wouldn't use that "not beautiful" (IMO) static factory "make" at all, If we are instantiating components... let's create them - aka "new Component() - why not? Why do we need to have one static method in one class able to instantiate one object of that class? Sounds a bit strange (and complex) in my OOP mind.

          So I think something like this:

          function single_button($singlebutton_or_array) {
          if ($singlebutton_or_array instance of single_button) {
                  $singlebutton = $singlebutton_or_array;
              } elseif (is_array($singlebutton_or_array)) {
                  $singlebutton = new single_button($singlebutton_or_array);
              } else {
                  throw new coding_exception('Must pass either single_button component or array');
              }
              return $this->render_singlebutton($singlebutton);
          

          Should be enough. So all the components constructors will be, always, one array, with some "clever" method able to apply all the setXXX(), detecting incorrect params and so on. And, in the long term... all the rendered methods should end allowing ONLY objects, with the array part becoming deprecated ASAP (I hate dual-behaviour methods like the one above).

          So, summarizing:

          1) I don't like long lists of params (that are prone to grow and grow... like current stable weblib has demonstrated along these years).
          2) I don't like "make" static methods to instantiate classes. Just instantiate them (new)
          3) I don't like "dual-behaviour" methods. In fact, there isn't much justification about having them in 2.0. As far as people already has to change all the weblib calls, differences are minimal (just put the "new xxxx" or no.
          4) Let's keep renderers having the control. Don't include any output in he component classes.

          Surely I'm saying some idiot things (due to my limited knowledge of the new renderers/components stuff). Just accept my comments as something coming from one developer still not having acquired "bad habits" (foul?) with that stuff, i.e. a "virginal pov". :-P

          Ciao

          Show
          Eloy Lafuente (stronk7) added a comment - Side comment1: Why the hell are we being renderer-centric instead of component-centric? I always thought that each component (box, button, list, table, header...) was going to support different renderers (core_renderer, workshop_renderer, cli_renderer, pdf_renderer...) instead of having each renderer implementing/overriding some/all the components. Obviously it's too late to change this. Just wanted to side comment it, because it can have implications in my comments below. Side comment2: IMO we are also having an important overlapping between formslib and outputlib/componentslib and, in the long term... that needs to be solved in a clever way. Else it's, once more, duplicate functionality and, worse, none of them are 100% successful (with formslib missing important rendering possibilities and componentslib missing all the (cool) validation stuff and so on). Side comment3: I'd delete all those "html_" prefixes from the components. Just let them to be "buttons", "checkboxes", "select menus"... whatever, but without being html ones. About the current issue: One of the (potentially) good things of having everything under the $OUTPUT umbrella is that I can, at any moment, create my own complete pdf renderer, text renderer... whatever, then replace $OUTPUT and, voilà, I'll have the page rendered in one different format. I think this is the approach followed by the cli_renderer, useful to perform installations and so on from terminal. In other words, $OUTPUT (the class), has total control over everything (the components and how they are finally rendered). I know this (complete text/pdf generation) is a stupid case right now, just the idea of the renderer being the boss is the key bit. If we use alternative 2, we are, for sure, losing that point, so the components themselves will be in charge of "rendering" the output. And that's nono (remember? we are (grrr) renderer-centric, so only renderers must be able to generate html (or text or pdf) code). So we end with alternatives 1 and 3, both pretty similar, and it's about to decide if we want to pass a long_list_of_params (like current stable weblib) or one component object created via factory method, well, I'd say NONE of them (nor both, as commented by David and Tim). In any case, I'd replace any long_list_of_params by one array_of_params (that avoids being adding params continuously). And in any case, I wouldn't use that "not beautiful" (IMO) static factory "make" at all, If we are instantiating components... let's create them - aka "new Component() - why not? Why do we need to have one static method in one class able to instantiate one object of that class? Sounds a bit strange (and complex) in my OOP mind. So I think something like this: function single_button($singlebutton_or_array) { if ($singlebutton_or_array instance of single_button) { $singlebutton = $singlebutton_or_array; } elseif (is_array($singlebutton_or_array)) { $singlebutton = new single_button($singlebutton_or_array); } else { throw new coding_exception('Must pass either single_button component or array'); } return $ this ->render_singlebutton($singlebutton); Should be enough. So all the components constructors will be, always, one array, with some "clever" method able to apply all the setXXX(), detecting incorrect params and so on. And, in the long term... all the rendered methods should end allowing ONLY objects, with the array part becoming deprecated ASAP (I hate dual-behaviour methods like the one above). So, summarizing: 1) I don't like long lists of params (that are prone to grow and grow... like current stable weblib has demonstrated along these years). 2) I don't like "make" static methods to instantiate classes. Just instantiate them (new) 3) I don't like "dual-behaviour" methods. In fact, there isn't much justification about having them in 2.0. As far as people already has to change all the weblib calls, differences are minimal (just put the "new xxxx" or no. 4) Let's keep renderers having the control. Don't include any output in he component classes. Surely I'm saying some idiot things (due to my limited knowledge of the new renderers/components stuff). Just accept my comments as something coming from one developer still not having acquired "bad habits" (foul?) with that stuff, i.e. a "virginal pov". :-P Ciao
          Hide
          Penny Leach added a comment -

          I like Eloy don't really know that much about what the API is like yet either, but here's what I think:

          • How much time does it take to learn the current API? Can we think of any way to cut that down?
          • Petr you commented that already people are commiting stuff to HEAD that isn't using the new API properly - can we mitigate this by more obviously splitting stuff into different files? Like put all the methods that people should use into a file, and everything else hidden somewhere with big NOT PUBLIC API messages at the top? Wiki pages are good but lots of us read the code and clear separation of what methods to use in one file and everything else somewhere else will probably help. ( and wiki pages too of course )
          • I tend to agree with David's example of a function that is flexible in what it accepts and also I don't like the static make functions much. Either pass a new object with all the complete things set up , or pass a few sensible parameters (as Tim stated earlier). Definitely agree about methods that have many many parameters, I hate that.
          • I am not sure about this component/renderer thing but if the purpose is like Eloy states, to have different renders to match different output types (html/xhtml/pdf/text/cli/xml/leap2a ( ) etc then clearly the html bit should go in the renderers , not in the components, and also html_ prefix should be removed as well.

          Hope that helps!

          Show
          Penny Leach added a comment - I like Eloy don't really know that much about what the API is like yet either, but here's what I think: How much time does it take to learn the current API? Can we think of any way to cut that down? Petr you commented that already people are commiting stuff to HEAD that isn't using the new API properly - can we mitigate this by more obviously splitting stuff into different files? Like put all the methods that people should use into a file, and everything else hidden somewhere with big NOT PUBLIC API messages at the top? Wiki pages are good but lots of us read the code and clear separation of what methods to use in one file and everything else somewhere else will probably help. ( and wiki pages too of course ) I tend to agree with David's example of a function that is flexible in what it accepts and also I don't like the static make functions much. Either pass a new object with all the complete things set up , or pass a few sensible parameters (as Tim stated earlier). Definitely agree about methods that have many many parameters, I hate that. I am not sure about this component/renderer thing but if the purpose is like Eloy states, to have different renders to match different output types (html/xhtml/pdf/text/cli/xml/leap2a ( ) etc then clearly the html bit should go in the renderers , not in the components, and also html_ prefix should be removed as well. Hope that helps!
          Hide
          David Mudrak added a comment -

          I always thought that each component (box, button, list, table, header...) was going to support different renderers (core_renderer, workshop_renderer, cli_renderer, pdf_renderer...) instead of having each renderer implementing/overriding some/all the components. – Eloy

          Component is just an object that describes the thing to be displayed. Scripts like view.php are responsible for preparing such objects. Then they are passed to a method of a renderer. Current theme decides which renderer is used (given that other factors influence this - like CLI mode). Module level renderers like workshop_renderer are not expected to render low level things like box, button, list, table or header. They provide methods to render high level things like posts, submissions, activity introduction etc. So it make sense to have

          $submission = workshop_get_submission();
          $workshopoutput = $PAGE->get_renderer('mod_workshop');
          echo $workshopoutput->submission($submission);
          

          but it does not make sense to ask $workshopoutput to render heading(), for example. Headings, boxes, lists, tables and other low level blocks should be rendered consistently within the current theme. We do not want allow modules to override these. That is IMHO why they went into core_renderer.

          And, in the long term... all the rendered methods should end allowing ONLY objects, with the array part becoming deprecated ASAP

          This may look as a nice design decission but it is not practical. For trivial components like headings or user pictures which has just two or three properties, explicit style is annoying

          $heading = new stdclass();
          $heading->text = 'Hello world';
          $heading->level = 2;
          echo $OUTPUT->heading($heading);
          

          compared with intuitive

          echo $OUTPUT->heading('Hello world', 2);
          

          In any case, I'd replace any long_list_of_params by one array_of_params

          I do not agree. Mandatory parameters (or frequently passed ones) should be real parameters with type constraints where possible. This allows IDE to display hints and type-control of the passed parameters. Other optional parameters can go into $options. I like Petr's way of doing this. Compare the following two rendering methods:

          public function avatar(array $properties) { }
          public function avatar($userid, $courseid=0, stdclass $user=null) { }
          

          IMO the second interface is self-documenting. IDEs are able to show you the list of method params while typing its name and I do not need to open the function description everytime just to read the list of keys acceptable via $properties array.

          Show
          David Mudrak added a comment - I always thought that each component (box, button, list, table, header...) was going to support different renderers (core_renderer, workshop_renderer, cli_renderer, pdf_renderer...) instead of having each renderer implementing/overriding some/all the components. – Eloy Component is just an object that describes the thing to be displayed. Scripts like view.php are responsible for preparing such objects. Then they are passed to a method of a renderer. Current theme decides which renderer is used (given that other factors influence this - like CLI mode). Module level renderers like workshop_renderer are not expected to render low level things like box, button, list, table or header. They provide methods to render high level things like posts, submissions, activity introduction etc. So it make sense to have $submission = workshop_get_submission(); $workshopoutput = $PAGE->get_renderer('mod_workshop'); echo $workshopoutput->submission($submission); but it does not make sense to ask $workshopoutput to render heading(), for example. Headings, boxes, lists, tables and other low level blocks should be rendered consistently within the current theme. We do not want allow modules to override these. That is IMHO why they went into core_renderer. And, in the long term... all the rendered methods should end allowing ONLY objects, with the array part becoming deprecated ASAP This may look as a nice design decission but it is not practical. For trivial components like headings or user pictures which has just two or three properties, explicit style is annoying $heading = new stdclass(); $heading->text = 'Hello world'; $heading->level = 2; echo $OUTPUT->heading($heading); compared with intuitive echo $OUTPUT->heading('Hello world', 2); In any case, I'd replace any long_list_of_params by one array_of_params I do not agree. Mandatory parameters (or frequently passed ones) should be real parameters with type constraints where possible. This allows IDE to display hints and type-control of the passed parameters. Other optional parameters can go into $options. I like Petr's way of doing this. Compare the following two rendering methods: public function avatar(array $properties) { } public function avatar($userid, $courseid=0, stdclass $user= null ) { } IMO the second interface is self-documenting. IDEs are able to show you the list of method params while typing its name and I do not need to open the function description everytime just to read the list of keys acceptable via $properties array.
          Hide
          Penny Leach added a comment -

          Yeah, I agree about $parameters or $properties being bad, I don't like that way at all

          Show
          Penny Leach added a comment - Yeah, I agree about $parameters or $properties being bad, I don't like that way at all
          Hide
          Sam Hemelryk added a comment -

          I agree with David and Penny when it comes to a properties array being used, I'm not a fan of that at all, every time you encounter a new function like that the only really good way to learn what is possible is to follow the coding path that the array takes.... but then I don't think that passing miles of parameters is the way either, as Eloy commented on parameters will always grow.
          In the example above trivial can quickly become not so, someone out there will give want to give headings classes, id's for styling or JS behaviours, any possible attribute. HTML5 which we will hopefully see within the lifetime of Moodle 2 will see the relevant list of attributes grow significantly).
          For small methods accepting n-args possibly; for many methods some sort of organisation or structure is surely required, which I thought was why we had components.
          I don't think DOM methods should be removed from the core_renderer, unless they are simply moved to a special renderer that core has access to, as I agree with Penny in that all HTML should pass through renderers.

          As for removing the `html_` prefix, sure why not less typing, but I don't think all components will be HTML components. I'm wasn't here when the output changers were being planned but from what I gathered from reading through the code and the conversion of weblib methods I gathered that components were simply grouped/reliant output. In the simplest example this could be a tag and content, in the more complex components a collection of tags, JavaScript actions and content.

          In general:

          • I thoroughly agree with Eloy's 4 summarising points.
          • I agree with Penny html from renderer not components
          • I could live without make methods, they are were only a convenience.
          • I am all for renaming things that contain an obvious/irrelevant prefix or suffix
          Show
          Sam Hemelryk added a comment - I agree with David and Penny when it comes to a properties array being used, I'm not a fan of that at all, every time you encounter a new function like that the only really good way to learn what is possible is to follow the coding path that the array takes.... but then I don't think that passing miles of parameters is the way either, as Eloy commented on parameters will always grow. In the example above trivial can quickly become not so, someone out there will give want to give headings classes, id's for styling or JS behaviours, any possible attribute. HTML5 which we will hopefully see within the lifetime of Moodle 2 will see the relevant list of attributes grow significantly). For small methods accepting n-args possibly; for many methods some sort of organisation or structure is surely required, which I thought was why we had components. I don't think DOM methods should be removed from the core_renderer, unless they are simply moved to a special renderer that core has access to, as I agree with Penny in that all HTML should pass through renderers. As for removing the `html_` prefix, sure why not less typing, but I don't think all components will be HTML components. I'm wasn't here when the output changers were being planned but from what I gathered from reading through the code and the conversion of weblib methods I gathered that components were simply grouped/reliant output. In the simplest example this could be a tag and content, in the more complex components a collection of tags, JavaScript actions and content. In general: I thoroughly agree with Eloy's 4 summarising points. I agree with Penny html from renderer not components I could live without make methods, they are were only a convenience. I am all for renaming things that contain an obvious/irrelevant prefix or suffix
          Hide
          Martin Dougiamas added a comment - - edited

          I'm going back and forth on the parameters issue. Thanks for all the great viewpoints posted so far, they are really helping.

          Some things I am pretty sure of:

          • I really don't like the idea of the first parameter being an object OR a string, etc. That's just messy.
          • It seems sensible to TRY and make this as close to the $DB API as possible.
          • I quite liked the way Tim was defining components/objects in view.php and then passing them. I think that was very self-documenting and made the code easy to debug. Readability is important in the code, as much as informing the IDE.
          • I do see how the component data structures can get more complex in the modules,
          • I don't think I like the make factory stuff. Bit too much magic.

          Overall I'm leaning towards something like this design:

          1) Always use one component object parameter for the render methods, all the time. The component do not need to be a full class with methods unless absolutely necessary, they might just be an array or simple data object.

          For simple cases:

          $mycomponent = new object();
          $mycomponent->something = "somevalue";
          $mycomponent->somethingcomplex = complex_thing_generator('a', 'b');
          echo $OUTPUT->somerenderer($mycomponent);
          

          and for more complex ones, if necessary:

          $mycomponent = new mycomponent();
          $mycomponent->set_properties('a','b','c','d');  // or however the component needs to be created
          echo $OUTPUT->somerenderer($mycomponent);
          

          2) For the simpler core render methods with only a few parameters, provide a consistently-labelled second short-hand function.

          echo $OUTPUT->somerenderer_simple('a','b','c','d');
          

          And this function just generates a component internally to pass to the real function in (1).

          Show
          Martin Dougiamas added a comment - - edited I'm going back and forth on the parameters issue. Thanks for all the great viewpoints posted so far, they are really helping. Some things I am pretty sure of: I really don't like the idea of the first parameter being an object OR a string, etc. That's just messy. It seems sensible to TRY and make this as close to the $DB API as possible. I quite liked the way Tim was defining components/objects in view.php and then passing them. I think that was very self-documenting and made the code easy to debug. Readability is important in the code, as much as informing the IDE. I do see how the component data structures can get more complex in the modules, I don't think I like the make factory stuff. Bit too much magic. Overall I'm leaning towards something like this design: 1) Always use one component object parameter for the render methods, all the time. The component do not need to be a full class with methods unless absolutely necessary, they might just be an array or simple data object. For simple cases: $mycomponent = new object(); $mycomponent->something = "somevalue" ; $mycomponent->somethingcomplex = complex_thing_generator('a', 'b'); echo $OUTPUT->somerenderer($mycomponent); and for more complex ones, if necessary: $mycomponent = new mycomponent(); $mycomponent->set_properties('a','b','c','d'); // or however the component needs to be created echo $OUTPUT->somerenderer($mycomponent); 2) For the simpler core render methods with only a few parameters, provide a consistently-labelled second short-hand function. echo $OUTPUT->somerenderer_simple('a','b','c','d'); And this function just generates a component internally to pass to the real function in (1).
          Hide
          David Mudrak added a comment -

          The issue was discussed in the chat room - see http://moodle.org/mod/cvsadmin/view.php?conversationid=3937
          David and Petr are going to prepare a wiki page with examples/tutorials for core devs, plugins/modules devs and (later) themers.

          Show
          David Mudrak added a comment - The issue was discussed in the chat room - see http://moodle.org/mod/cvsadmin/view.php?conversationid=3937 David and Petr are going to prepare a wiki page with examples/tutorials for core devs, plugins/modules devs and (later) themers.
          Hide
          Martin Dougiamas added a comment -
          Show
          Martin Dougiamas added a comment - For reference: http://docs.moodle.org/en/Development:Output_renderers
          Hide
          Petr Škoda added a comment -

          mostly converted, the output api should have a final coding style

          Show
          Petr Škoda added a comment - mostly converted, the output api should have a final coding style
          Hide
          Martin Dougiamas added a comment - - edited

          Great to hear! Can you please make sure you document it fully at http://docs.moodle.org/en/Development:Output_renderers ?

          We need something to point all developers to.

          Show
          Martin Dougiamas added a comment - - edited Great to hear! Can you please make sure you document it fully at http://docs.moodle.org/en/Development:Output_renderers ? We need something to point all developers to.

            People

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

              Dates

              • Created:
                Updated:
                Resolved: