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

Allow renderers and renderables in a namespace to be auto loaded.

    Details

      Description

      For new plugins at least - it would be nice to be able to put the renderer in a class in the plugins classes folder and use a namespace instead of the full frankenstyle name for the class. Same goes for renderables - they currently can't be put in a namespace.

      Current situation:

      Currently we look for renderer class like this (depending on the renderer_factory):

      standard_renderer_factory

      (get_renderer called with $component = 'mod_assign', $subtype = 'custom', $target='ajax')

      Search list:

      mod_assign_custom_renderer_ajax
      mod_assign_custom_renderer
      

      theme_overridden_renderer_factory

      (get_renderer called with $component = 'mod_assign', $subtype = 'custom', $target='ajax' and theme_clean inherits from theme_bootstrapbase)

      Search list:

      theme_clean_mod_assign_custom_renderer_ajax
      theme_bootstrapbase_mod_assign_custom_renderer_ajax
      mod_assign_custom_renderer_ajax
      theme_clean_mod_assign_custom_renderer
      theme_bootstrapbase_mod_assign_custom_renderer
      mod_assign_custom_renderer
      

      renderables

      And currently renderables cannot existing in a namespace, because the class name is used in the render_xxx method of the renderer.

      After this patch:

      standard_renderer_factory

      (get_renderer called with $component = 'mod_assign', $subtype = 'custom', $target='ajax')

      Search list:

      \\mod_assign\\output\\custom_renderer_ajax
      \\mod_assign\\output\\custom_renderer
      mod_assign_custom_renderer_ajax
      mod_assign_custom_renderer
      

      theme_overridden_renderer_factory

      (get_renderer called with $component = 'mod_assign', $subtype = 'custom', $target='ajax' and theme_clean inherits from theme_bootstrapbase)

      Search list:

      theme_clean\\output\\mod_assign\\custom_renderer_ajax
      theme_clean_mod_assign_custom_renderer_ajax
      theme_bootstrapbase\\output\\mod_assign\\custom_renderer_ajax
      theme_bootstrapbase_mod_assign_custom_renderer_ajax
      \\mod_assign\\output\\custom_renderer_ajax
      mod_assign_custom_renderer_ajax
      theme_clean\\output\\mod_assign\\custom_renderer
      theme_clean_mod_assign_custom_renderer
      theme_bootstrapbase\\output\\mod_assign\\custom_renderer
      theme_bootstrapbase_mod_assign_custom_renderer
      \\mod_assign\\output\\custom_renderer
      mod_assign_custom_renderer
      

      renderables

      renderables can exist in the (e.g.) \mod_assign\output\test_renderable class, and the render method on the renderer will be "render_test" (_renderable is removed).

        Gliffy Diagrams

          Issue Links

            Activity

            Hide
            damyon Damyon Wiese added a comment - - edited

            Here is a patch that allows:

            renderers to be defined in the XXX\renderer namespace (e.g. mod_assign\renderer\renderer for the default renderer for mod_assign)

            renderers to be defined in both places (so we can move renderers to the autoloaded place, but leave a stub for them in the old location for backwards compatibility)

            an namespaced renderer can override a non-namespaced one (e.g. theme_clean\renderer\core\renderer.php overrides core_renderer)

            Comments: there may be performance concerns about the extra searching for classnames etc - it would be possible to cache the results of these searches - e.g. in a json file in the dataroot - but I bet that would be slower because php would already be caching the results of the class_exists itself (especially with opcache). I "lamo" tested the performance just be checking the time to render in the footer before and after this patch and didn't see any difference.

            Show
            damyon Damyon Wiese added a comment - - edited Here is a patch that allows: renderers to be defined in the XXX\renderer namespace (e.g. mod_assign\renderer\renderer for the default renderer for mod_assign) renderers to be defined in both places (so we can move renderers to the autoloaded place, but leave a stub for them in the old location for backwards compatibility) an namespaced renderer can override a non-namespaced one (e.g. theme_clean\renderer\core\renderer.php overrides core_renderer) Comments: there may be performance concerns about the extra searching for classnames etc - it would be possible to cache the results of these searches - e.g. in a json file in the dataroot - but I bet that would be slower because php would already be caching the results of the class_exists itself (especially with opcache). I "lamo" tested the performance just be checking the time to render in the footer before and after this patch and didn't see any difference.
            Hide
            damyon Damyon Wiese added a comment - - edited

            Note - this patch has 3 commits. The first is the only one intended for integration, the second 2 are just for proving it works.

            Show
            damyon Damyon Wiese added a comment - - edited Note - this patch has 3 commits. The first is the only one intended for integration, the second 2 are just for proving it works.
            Hide
            damyon Damyon Wiese added a comment -

            To describe this change - previously all renderers and renderables were excluded from auto-loading because some functions where assuming a single class name created by joining strings with '_'.

            With this patch, renderers can be put in the autoloaded <pluginname>/classes/output/ folder (and must be "renderer" for the default renderer or "xxxx_renderer" for the xxxx_subtype)

            Renderables must go in the same folder "<pluginname>/classes/output/" and end with _renderable.

            A theme can override a renderer by placing a class in: <themefolder>/classes/output/<component_name>/renderer.php (or xxxx_renderer.php).

            An autoloaded renderer can override a non-autoloaded renderer.

            A class can exist in both the old and new locations/names (so we can move renderers and maintain a stub for backwards compatibility).

            Show
            damyon Damyon Wiese added a comment - To describe this change - previously all renderers and renderables were excluded from auto-loading because some functions where assuming a single class name created by joining strings with '_'. With this patch, renderers can be put in the autoloaded <pluginname>/classes/output/ folder (and must be "renderer" for the default renderer or "xxxx_renderer" for the xxxx_subtype) Renderables must go in the same folder "<pluginname>/classes/output/" and end with _renderable. A theme can override a renderer by placing a class in: <themefolder>/classes/output/<component_name>/renderer.php (or xxxx_renderer.php). An autoloaded renderer can override a non-autoloaded renderer. A class can exist in both the old and new locations/names (so we can move renderers and maintain a stub for backwards compatibility).
            Hide
            cibot CiBoT added a comment -

            Results for MDL-41663

            • Remote repository: git://github.com/damyon/moodle.git
            Show
            cibot CiBoT added a comment - Results for MDL-41663 Remote repository: git://github.com/damyon/moodle.git Remote branch MDL-41663 -master to be integrated into upstream master Executed job http://integration.moodle.org/job/Precheck%20remote%20branch/3373 Details: http://integration.moodle.org/job/Precheck%20remote%20branch/3373/artifact/work/smurf.html
            Hide
            ankit_frenz Ankit Agarwal added a comment - - edited

            Hi Damyon,
            Thanks for working on this issue. Does look good.
            A few surrounding things that will need to be discussed/done before this can be integrated:-

            1. Load testing (Not sure if there would be much change though, since we catch a list of all auto-loadable class names)
            2. Discussion in forums about this change
            3. Follow up issues to update renderers in standard plugins/core
            4. My vote would be to put renderable and renderer in separate namespaces, so we can separate view classes from everything else.

            About the patch:-
            standard_renderer_classnames

            1. standard_renderer_classnames could do with some unit tests
            2. Unrequired extra line at L192
            3. phpdoc for $subtype (standard_renderer_classnames()) needs to be updated to reflect the changes

            standard_renderer_factory::get_renderer

            1. Now the api will throw a php fatal error instead of coding exception if the renderer doesn't exist. Any reason why we should change the old behaviour?

            Rest looks good.

            Show
            ankit_frenz Ankit Agarwal added a comment - - edited Hi Damyon, Thanks for working on this issue. Does look good. A few surrounding things that will need to be discussed/done before this can be integrated:- Load testing (Not sure if there would be much change though, since we catch a list of all auto-loadable class names) Discussion in forums about this change Follow up issues to update renderers in standard plugins/core My vote would be to put renderable and renderer in separate namespaces, so we can separate view classes from everything else. About the patch:- standard_renderer_classnames standard_renderer_classnames could do with some unit tests Unrequired extra line at L192 phpdoc for $subtype (standard_renderer_classnames()) needs to be updated to reflect the changes standard_renderer_factory::get_renderer Now the api will throw a php fatal error instead of coding exception if the renderer doesn't exist. Any reason why we should change the old behaviour? Rest looks good.
            Hide
            damyon Damyon Wiese added a comment -

            Thanks Ankit.

            1. Yep - since the autoload list is cached there does not seem to be much impact. I can run the perf test on this before and after - (I will have to remember how to do it)
            2. Will do - but I want to iron out kicks and propose a working solution there (it will make the discussion more efficient).
            3. Why ? the old renderers don't need moving. It's the same as the classloading in general - there's no point in updating all the old code unless we are rewriting it for some other reason. New code should use this, or places where the maintainer wants to make the effort.
            4. E.g. in a theme overridden renderer, the name space is already 3 levels. 4 seems excessive (theme_clean\output\mod_assign\renderable\grading_summary vs theme_clean\output\mod_assign\grading_summary_renderable). Besides I have plans to move the actual renderers completely to templates which will do what you want anyway.

            1. Ok - will do
            2. Will remove
            3. Will do thanks.

            1. I'll check - it should be the same behaviour as before.

            Show
            damyon Damyon Wiese added a comment - Thanks Ankit. 1. Yep - since the autoload list is cached there does not seem to be much impact. I can run the perf test on this before and after - (I will have to remember how to do it) 2. Will do - but I want to iron out kicks and propose a working solution there (it will make the discussion more efficient). 3. Why ? the old renderers don't need moving. It's the same as the classloading in general - there's no point in updating all the old code unless we are rewriting it for some other reason. New code should use this, or places where the maintainer wants to make the effort. 4. E.g. in a theme overridden renderer, the name space is already 3 levels. 4 seems excessive (theme_clean\output\mod_assign\renderable\grading_summary vs theme_clean\output\mod_assign\grading_summary_renderable). Besides I have plans to move the actual renderers completely to templates which will do what you want anyway. 1. Ok - will do 2. Will remove 3. Will do thanks. 1. I'll check - it should be the same behaviour as before.
            Hide
            timhunt Tim Hunt added a comment -

            2. You don't think peopel in the communtiy might have something to contribute to ironing out the kinks in the proposal?
            4. Your response makes no sense. Themes only override renderers, not renderables so not chagne there. I think we are talking about having \mod_assign\render\renderer.php and \mod_assign\renderable\submission.php. Seems nice to me.

            Show
            timhunt Tim Hunt added a comment - 2. You don't think peopel in the communtiy might have something to contribute to ironing out the kinks in the proposal? 4. Your response makes no sense. Themes only override renderers, not renderables so not chagne there. I think we are talking about having \mod_assign\render\renderer.php and \mod_assign\renderable\submission.php. Seems nice to me.
            Hide
            damyon Damyon Wiese added a comment -

            2. I am still testing this and found and fixed some bugs. There is plenty of time and this won't get pushed without getting feedback.

            Show
            damyon Damyon Wiese added a comment - 2. I am still testing this and found and fixed some bugs. There is plenty of time and this won't get pushed without getting feedback.
            Hide
            damyon Damyon Wiese added a comment -

            4. It cannot be \mod_assign\render\renderer.php and \mod_assign\renderable\submission.php because that is a violation of the namespacing rules. This is part of the "output" api so that must be used for the 2nd level. This would be:
            \mod_assign\output\renderable\submission.php
            \mod_assign\output\renderer\submission.php

            And also - there would almost always only be a single renderer for the plugin that renders all the renderables. So mod_assign\output\renderer would almost always contain only one class.

            Show
            damyon Damyon Wiese added a comment - 4. It cannot be \mod_assign\render\renderer.php and \mod_assign\renderable\submission.php because that is a violation of the namespacing rules. This is part of the "output" api so that must be used for the 2nd level. This would be: \mod_assign\output\renderable\submission.php \mod_assign\output\renderer\submission.php And also - there would almost always only be a single renderer for the plugin that renders all the renderables. So mod_assign\output\renderer would almost always contain only one class.
            Hide
            damyon Damyon Wiese added a comment -

            And in the theme - to override the renderer it would be worse:
            theme_clean\output\mod_assign\renderer\renderer.php
            vs
            theme_clean\output\mod_assign\renderer.php

            Show
            damyon Damyon Wiese added a comment - And in the theme - to override the renderer it would be worse: theme_clean\output\mod_assign\renderer\renderer.php vs theme_clean\output\mod_assign\renderer.php
            Hide
            damyon Damyon Wiese added a comment -

            Updated the branch - I moved the testing commits to separate branches and updated the testing instructions.

            Since Ankits review I have:

            • added a unit test
            • removed the unused line
            • fixed the phpdoc
            Show
            damyon Damyon Wiese added a comment - Updated the branch - I moved the testing commits to separate branches and updated the testing instructions. Since Ankits review I have: added a unit test removed the unused line fixed the phpdoc
            Hide
            damyon Damyon Wiese added a comment -

            Posted to the developers forum here:
            https://moodle.org/mod/forum/discuss.php?d=260606

            Show
            damyon Damyon Wiese added a comment - Posted to the developers forum here: https://moodle.org/mod/forum/discuss.php?d=260606
            Hide
            dobedobedoh Andrew Nicols added a comment -

            I like this idea - it's something that would make my work on mod_forum a little easier.
            I take it that this will play nicely with MDL-43365 and then we can also deprecate and move renderers to new locations where it's appropriate? mod_forum has one renderer with three functions and it would be frustrating to not be able to move it to a new location just because they may be overridden in a theme somewhere.

            Cheers,

            Andrew

            Show
            dobedobedoh Andrew Nicols added a comment - I like this idea - it's something that would make my work on mod_forum a little easier. I take it that this will play nicely with MDL-43365 and then we can also deprecate and move renderers to new locations where it's appropriate? mod_forum has one renderer with three functions and it would be frustrating to not be able to move it to a new location just because they may be overridden in a theme somewhere. Cheers, Andrew
            Hide
            damyon Damyon Wiese added a comment -

            Hi Andrew,

            Yes this will work with MDL-43365 - and you'll need both issues together in order to move an existing renderer and maintain backwards compatibility for themers.

            Show
            damyon Damyon Wiese added a comment - Hi Andrew, Yes this will work with MDL-43365 - and you'll need both issues together in order to move an existing renderer and maintain backwards compatibility for themers.
            Hide
            damyon Damyon Wiese added a comment -

            More details on that last comment, because we search for the auto loaded name first - if there are both, the debugging from the class aliasing wont get triggered.

            Show
            damyon Damyon Wiese added a comment - More details on that last comment, because we search for the auto loaded name first - if there are both, the debugging from the class aliasing wont get triggered.
            Hide
            timhunt Tim Hunt added a comment -

            I know that my proposed naming convention breaks the current rule for namspacing, but

            • If an arbitrary rule is getting in the way, we should review it.
            • Output is a very vague and amorphous concept, it might help the design of Moodle more generally to split out a Renderer API and a Renderable API. Worth considering. Leave the Output API for things like html_writer, s, format_string and format_text.

            Also, I am not sure that it is right to say that most add-ons will only have one renderer. Well, most will, but in that case they are probably small add-ons, and it might be better to not use namespacing. Just use the facility to put the renderer directly in classes.

            But for big, complex modules, and clearly I am being influenced by my experience of quiz, one renderer is a nightmare. mod/quiz/renderer.php is way too big for a single file.

            What I am considering doing (and I am not sure, which is why I put it here for discussion) is to split the quiz renderer into:

            • mod_quiz_renderer, which outputs the common quiz-specific elements that appear on many pages in the quiz.
            • mod_quiz_ {page_name}

              _renderer (where page name is view, start_attempt, attempt, summary, review, index, report, edit), for those elements that are used only on that particular page. See http://docs.moodle.org/dev/Quiz_user_interface_overview if you are not familiar with the quiz.
              At the moment, that is my thinking for breaking the one huge file into bits. Comments welcome.

            Show
            timhunt Tim Hunt added a comment - I know that my proposed naming convention breaks the current rule for namspacing, but If an arbitrary rule is getting in the way, we should review it. Output is a very vague and amorphous concept, it might help the design of Moodle more generally to split out a Renderer API and a Renderable API. Worth considering. Leave the Output API for things like html_writer, s, format_string and format_text. Also, I am not sure that it is right to say that most add-ons will only have one renderer. Well, most will, but in that case they are probably small add-ons, and it might be better to not use namespacing. Just use the facility to put the renderer directly in classes. But for big, complex modules, and clearly I am being influenced by my experience of quiz, one renderer is a nightmare. mod/quiz/renderer.php is way too big for a single file. What I am considering doing (and I am not sure, which is why I put it here for discussion) is to split the quiz renderer into: mod_quiz_renderer, which outputs the common quiz-specific elements that appear on many pages in the quiz. mod_quiz_ {page_name} _renderer (where page name is view, start_attempt, attempt, summary, review, index, report, edit), for those elements that are used only on that particular page. See http://docs.moodle.org/dev/Quiz_user_interface_overview if you are not familiar with the quiz. At the moment, that is my thinking for breaking the one huge file into bits. Comments welcome.
            Hide
            damyon Damyon Wiese added a comment - - edited

            Tim - with this patch you would put those at:

            \\mod_quiz\\output\\{page_name}_renderer
            

            Which seems neat and tidy.

            Or you could leave them out of a namespace and use

            mod_quiz_{page_name}_renderer 
            

            (current method).

            Show
            damyon Damyon Wiese added a comment - - edited Tim - with this patch you would put those at: \\mod_quiz\\output\\{page_name}_renderer Which seems neat and tidy. Or you could leave them out of a namespace and use mod_quiz_{page_name}_renderer (current method).
            Hide
            timhunt Tim Hunt added a comment -

            Damyon, lots of things work, including the status quo. Here we are talking about what would work best in future. If a typical modules has a dozen renderers and 20 or so renderables, then mising them in one folder, particularly with a naming convention that will cause the two sets of things to jumble up, does not seem like a good solution to me.

            Show
            timhunt Tim Hunt added a comment - Damyon, lots of things work, including the status quo. Here we are talking about what would work best in future. If a typical modules has a dozen renderers and 20 or so renderables, then mising them in one folder, particularly with a naming convention that will cause the two sets of things to jumble up, does not seem like a good solution to me.
            Hide
            damyon Damyon Wiese added a comment -

            Sure Tim - please suggest an alternative that does not violate the namespacing rules.

            Show
            damyon Damyon Wiese added a comment - Sure Tim - please suggest an alternative that does not violate the namespacing rules.
            Hide
            timhunt Tim Hunt added a comment -

            I did, above. Subdivide the wide diversity of things we currently collect togeter under the name output API into

            • Renderers API - what is currently in lib/outputrenderers, and probably outputfactories.php.
            • Renderables API - what is currently in lib/outputrenderers.php, and outputactions.php if we want to keep that.
            • Output API - low-level output generation, like html_writer, s, format_string, format_text, and other appropriate parts of weblib.php
            • Theme API - core handling of theme plugins. What is in lib/outputlib.php, and

            Someone should go through all of weblib.php and work out which APIs all the bits belong to.

            Acuatlly, lib/classes is the strongest argouemtn for splitting this into a few smaller APIs, otherwise lib/classes/output will be a huge mess.

            That 4-way split seems to work quite nicely. I was not sure how well it would work out when I started writing.

            Show
            timhunt Tim Hunt added a comment - I did, above. Subdivide the wide diversity of things we currently collect togeter under the name output API into Renderers API - what is currently in lib/outputrenderers, and probably outputfactories.php. Renderables API - what is currently in lib/outputrenderers.php, and outputactions.php if we want to keep that. Output API - low-level output generation, like html_writer, s, format_string, format_text, and other appropriate parts of weblib.php Theme API - core handling of theme plugins. What is in lib/outputlib.php, and Someone should go through all of weblib.php and work out which APIs all the bits belong to. Acuatlly, lib/classes is the strongest argouemtn for splitting this into a few smaller APIs, otherwise lib/classes/output will be a huge mess. That 4-way split seems to work quite nicely. I was not sure how well it would work out when I started writing.
            Hide
            davosmith Davo Smith added a comment -

            This sounds like a great idea.
            On the 'separate namespaces for renderers and renderables' issue, would this end up with code that looked like this:

            function render_mywidget(\mod_mymod\output\renderable\mywidget $mywidget)  {
            

            Instead of:

            function render_mywidget(mywidget $mywidget) {
            

            For clarity, I'd prefer to be writing the latter (and making use of the shared namespace) - or am I showing off my currently limited understanding of namespaces (my clients are only just moving on to 2.6, so I've only just started using the auto class loading in Moodle).

            Show
            davosmith Davo Smith added a comment - This sounds like a great idea. On the 'separate namespaces for renderers and renderables' issue, would this end up with code that looked like this: function render_mywidget(\mod_mymod\output\renderable\mywidget $mywidget) { Instead of: function render_mywidget(mywidget $mywidget) { For clarity, I'd prefer to be writing the latter (and making use of the shared namespace) - or am I showing off my currently limited understanding of namespaces (my clients are only just moving on to 2.6, so I've only just started using the auto class loading in Moodle).
            Hide
            timhunt Tim Hunt added a comment -

            You can say

            use \mod_mymod\output\renderable\mywidget;
            

            at the top of the PHP file.

            Show
            timhunt Tim Hunt added a comment - You can say use \mod_mymod\output\renderable\mywidget; at the top of the PHP file.
            Hide
            damyon Damyon Wiese added a comment - - edited

            I have to say - I think if we start splitting our APIs into super specific categories like:
            Renderers API, Renderables API, Output API, Theme API we will have hundreds of APIs in a flat hierarchy. These things all sit in the Output API in my view - and they are all related.

            If we want to split the namespaces for renderers and renderables that's fine - I think we'll have to do it under the "output" namespace (as noted above).

            So you would have:

            mod_assign\output\renderer\renderer.php (the default)
            

            and

            mod_assign\output\renderer\{page_type}.php (a page type specific renderer)
            

            which can be overridden by

            theme_bootstrapbase\\output\\mod_assign\\renderer\\{page_type}.php
            

            If we agree that this is not namespace overload then it's fine by me.

            Show
            damyon Damyon Wiese added a comment - - edited I have to say - I think if we start splitting our APIs into super specific categories like: Renderers API, Renderables API, Output API, Theme API we will have hundreds of APIs in a flat hierarchy. These things all sit in the Output API in my view - and they are all related. If we want to split the namespaces for renderers and renderables that's fine - I think we'll have to do it under the "output" namespace (as noted above). So you would have: mod_assign\output\renderer\renderer.php (the default) and mod_assign\output\renderer\{page_type}.php (a page type specific renderer) which can be overridden by theme_bootstrapbase\\output\\mod_assign\\renderer\\{page_type}.php If we agree that this is not namespace overload then it's fine by me.
            Hide
            mudrd8mz David Mudrak added a comment -

            Tim, I must agree with Damyon here. For most plugins, one render is fully ok imho and it does not need to be a "small plugin". Quiz is big and complex, for most plugins (and especially contributed add-ons), we will be happy to see at least some renderer.

            My -1 to further break down output API into smaller ones. Renderers and renderables are natural part of one world. Neither splitting them to two separate API nor namespaces makes much sense to me just off my mind now, but maybe I just can't see far enough to realize all consequences.

            One thing that is not clear to me: is the location of the non-namespaced code going to change to the root of the classes folder (as I caught somewhere above) or is it just going to stay as is now? We need BC anyway so ...

            Show
            mudrd8mz David Mudrak added a comment - Tim, I must agree with Damyon here. For most plugins, one render is fully ok imho and it does not need to be a "small plugin". Quiz is big and complex, for most plugins (and especially contributed add-ons), we will be happy to see at least some renderer. My -1 to further break down output API into smaller ones. Renderers and renderables are natural part of one world. Neither splitting them to two separate API nor namespaces makes much sense to me just off my mind now, but maybe I just can't see far enough to realize all consequences. One thing that is not clear to me: is the location of the non-namespaced code going to change to the root of the classes folder (as I caught somewhere above) or is it just going to stay as is now? We need BC anyway so ...
            Hide
            damyon Damyon Wiese added a comment -

            The non-namespaced version can either be in the root of the classes folder and will be found by autoloading - or in the old location (renderer.php).

            This is fully backwards compatible.

            Show
            damyon Damyon Wiese added a comment - The non-namespaced version can either be in the root of the classes folder and will be found by autoloading - or in the old location (renderer.php). This is fully backwards compatible.
            Hide
            damyon Damyon Wiese added a comment -

            rebased...

            Show
            damyon Damyon Wiese added a comment - rebased...
            Hide
            damyon Damyon Wiese added a comment -

            Noting that while Tim still disagrees with this approach because the renderers and renderables go in the same folder - I think it is the correct solution and am going to push it forward soon. It will block work on the element library.

            I have sought feedback on the forums, and in the dev chat. I'll leave it up to the integrator to agree, disagree, delay this further.

            Show
            damyon Damyon Wiese added a comment - Noting that while Tim still disagrees with this approach because the renderers and renderables go in the same folder - I think it is the correct solution and am going to push it forward soon. It will block work on the element library. I have sought feedback on the forums, and in the dev chat. I'll leave it up to the integrator to agree, disagree, delay this further.
            Hide
            cibot CiBoT added a comment -

            Moving this issue to current integration cycle, will be reviewed soon. Thanks for the hard work!

            Show
            cibot CiBoT added a comment - Moving this issue to current integration cycle, will be reviewed soon. Thanks for the hard work!
            Hide
            stronk7 Eloy Lafuente (stronk7) added a comment -

            Hi guys,

            as far as this was sent to forums recently I'm going to keep it under integration 1 more week. I think it's ok, just in case somebody comes with a great/best idea.

            Said that, my personal opinion is that I like current implementation and don't like to add new (out of law) level2 namespaces at all. The only alternative I'd to comment, for your consideration is if it would be interesting to allow something like this:

            standard_renderer_factory

            (get_renderer called with $component = 'mod_assign', $subtype = 'custom', $target='ajax')

            Search list:

            \\mod_assign\\output\\custom_renderer_ajax
            \\mod_assign\\output\\custom_renderer
            \\mod_assign\\output\\custom\\renderer_ajax
            \\mod_assign\\output\\custom\\renderer
            mod_assign_custom_renderer_ajax
            mod_assign_custom_renderer
            

            Basically, expand a bit the search so the $subtype can become a level 3 namespace. That will help for components having tons of classes, giving some extra organization freedom to the developer without breaking the level1 (component) and level2 (api) rules.

            Ciao

            Show
            stronk7 Eloy Lafuente (stronk7) added a comment - Hi guys, as far as this was sent to forums recently I'm going to keep it under integration 1 more week. I think it's ok, just in case somebody comes with a great/best idea. Said that, my personal opinion is that I like current implementation and don't like to add new (out of law) level2 namespaces at all. The only alternative I'd to comment, for your consideration is if it would be interesting to allow something like this: standard_renderer_factory (get_renderer called with $component = 'mod_assign', $subtype = 'custom', $target='ajax') Search list: \\mod_assign\\output\\custom_renderer_ajax \\mod_assign\\output\\custom_renderer \\mod_assign\\output\\custom\\renderer_ajax \\mod_assign\\output\\custom\\renderer mod_assign_custom_renderer_ajax mod_assign_custom_renderer Basically, expand a bit the search so the $subtype can become a level 3 namespace. That will help for components having tons of classes, giving some extra organization freedom to the developer without breaking the level1 (component) and level2 (api) rules. Ciao
            Hide
            damyon Damyon Wiese added a comment -

            Thanks Eloy - np delaying - that extra searching sounds fine to me - I'll update the branch for next week.

            Show
            damyon Damyon Wiese added a comment - Thanks Eloy - np delaying - that extra searching sounds fine to me - I'll update the branch for next week.
            Hide
            damyon Damyon Wiese added a comment -

            Updated the branch to allow that extra level of namespacing.

            I also fixed the order in the unit test so the fixture matches the real search order for the standard renderer factory (always searches for all the suffixed versions first).

            Show
            damyon Damyon Wiese added a comment - Updated the branch to allow that extra level of namespacing. I also fixed the order in the unit test so the fixture matches the real search order for the standard renderer factory (always searches for all the suffixed versions first).
            Hide
            timhunt Tim Hunt added a comment -

            I think the extra level of name-spaces is a really stupid breaking of the rules.

            If you think that, after all, too many classes in one folder will become a problem, then fix that problem properly by getting good APIs/namespaces. Don't add a hack here that it outside all the existing rules.

            Or just don't worry about it. Stick to your original plan.

            Show
            timhunt Tim Hunt added a comment - I think the extra level of name-spaces is a really stupid breaking of the rules. If you think that, after all, too many classes in one folder will become a problem, then fix that problem properly by getting good APIs/namespaces. Don't add a hack here that it outside all the existing rules. Or just don't worry about it. Stick to your original plan.
            Hide
            damyon Damyon Wiese added a comment -

            What rule is it breaking Tim ?

            Show
            damyon Damyon Wiese added a comment - What rule is it breaking Tim ?
            Hide
            timhunt Tim Hunt added a comment -

            The namespace rules that the integrators brokered agreement on weeks ago, and which they still have not written up in the Coding Style docs. https://tracker.moodle.org/browse/MDLSITE-2549 / http://docs.moodle.org/dev/User:Eloy_Lafuente_(stronk7)/Namespaces

            Show
            timhunt Tim Hunt added a comment - The namespace rules that the integrators brokered agreement on weeks ago, and which they still have not written up in the Coding Style docs. https://tracker.moodle.org/browse/MDLSITE-2549 / http://docs.moodle.org/dev/User:Eloy_Lafuente_(stronk7)/Namespaces
            Hide
            damyon Damyon Wiese added a comment -

            It doesn't break those rules at all Tim. Level 3 namespaces have no rules.

            Show
            damyon Damyon Wiese added a comment - It doesn't break those rules at all Tim. Level 3 namespaces have no rules.
            Hide
            timhunt Tim Hunt added a comment -

            But, you are proposing to add rules for renderer subtypes now, just based on one off-hand comment from Eloy (from what I can see).

            How will anyone discover this? What problem does it solve? Do we want any rules at level 3?

            Show
            timhunt Tim Hunt added a comment - But, you are proposing to add rules for renderer subtypes now, just based on one off-hand comment from Eloy (from what I can see). How will anyone discover this? What problem does it solve? Do we want any rules at level 3?
            Hide
            damyon Damyon Wiese added a comment -

            There are already rules for renderer subtypes - this is just relaxing those rules so you can put them in a 3rd level namespace (or not).

            I can imaging different styles of code using namespaces heavily or not at all which is what we are trying to allow here.

            Show
            damyon Damyon Wiese added a comment - There are already rules for renderer subtypes - this is just relaxing those rules so you can put them in a 3rd level namespace (or not). I can imaging different styles of code using namespaces heavily or not at all which is what we are trying to allow here.
            Hide
            stronk7 Eloy Lafuente (stronk7) added a comment -

            Wow Tim, I proposed it thinking on you, really. I just thought it could be a "natural" way to allow more classification/spread into subdirectories without breaking the rules. That seemed to be your main concern (or at least that's what I understood).

            Show
            stronk7 Eloy Lafuente (stronk7) added a comment - Wow Tim, I proposed it thinking on you, really. I just thought it could be a "natural" way to allow more classification/spread into subdirectories without breaking the rules. That seemed to be your main concern (or at least that's what I understood).
            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
            timhunt Tim Hunt added a comment -

            The way it looks to me, Eloy, is we had two sensible logical proposals, A and B, which followed all the namespacing rules. You and Damyon though A was better, I thought B was better, and we had a long debate about that.

            Then, suddenly, and with minimal reflection and debate, you decide to add a hack to make up a new option A--, which superficially addresses the one way in which B is better than A, but in a nasty way.

            You now claim that A-- is the best option, but actually it is the worst.

            Really, just go back to your patch. Or, start a new thread in the forums, if you think that A-- is really better, and can explain why.

            Show
            timhunt Tim Hunt added a comment - The way it looks to me, Eloy, is we had two sensible logical proposals, A and B, which followed all the namespacing rules. You and Damyon though A was better, I thought B was better, and we had a long debate about that. Then, suddenly, and with minimal reflection and debate, you decide to add a hack to make up a new option A--, which superficially addresses the one way in which B is better than A, but in a nasty way. You now claim that A-- is the best option, but actually it is the worst. Really, just go back to your patch. Or, start a new thread in the forums, if you think that A-- is really better, and can explain why.
            Hide
            stronk7 Eloy Lafuente (stronk7) added a comment - - edited

            I think I already have explained its benefits, but will be doing it again:

            1) It's optional. Exactly the same type of option that non-namespaces alternative offer.
            2) It's not against the rules (observes level1 and level2 rules, plus gives level3 freedom, also under the rules).
            3) It helps to avoid "mass use" of level2 (a problem you invented, btw).

            Said that, not sure how/why you are assuming that any of these is true, because none of them is correct:

            a) it's hack. No it's not. It's an agreement about how to allow the output API to use namespaces.
            b) with minimal reflection. Just because I prefer to listen and learn before commenting, you're 100% wrong here, I'm sorry.
            c) superficially addresses the "mass use". It solves the problem completely, or how is it not solved?
            d) in a nasty way. Can you say why the way is nasty?
            e) is the worst. No, clearly your one was the worst (for me). The simple fact of breaking the rules fully disqualifies it.

            In any case, I'm happy with current patch, as I commented above. I don't have any problem having 40 classes in a directory, at all.

            I just suggested (not "decided", I think I said "for your consideration" and that has a meaning, at least in Spanish) a viable alternative bringing one extra level of optional organization to developers worried about proliferation of classes in a single dir.

            Ciao

            Edited, to show the difference between decide and suggest.

            Show
            stronk7 Eloy Lafuente (stronk7) added a comment - - edited I think I already have explained its benefits, but will be doing it again: 1) It's optional. Exactly the same type of option that non-namespaces alternative offer. 2) It's not against the rules (observes level1 and level2 rules, plus gives level3 freedom, also under the rules). 3) It helps to avoid "mass use" of level2 (a problem you invented, btw). Said that, not sure how/why you are assuming that any of these is true, because none of them is correct: a) it's hack. No it's not. It's an agreement about how to allow the output API to use namespaces. b) with minimal reflection. Just because I prefer to listen and learn before commenting, you're 100% wrong here, I'm sorry. c) superficially addresses the "mass use". It solves the problem completely, or how is it not solved? d) in a nasty way. Can you say why the way is nasty? e) is the worst. No, clearly your one was the worst (for me). The simple fact of breaking the rules fully disqualifies it. In any case, I'm happy with current patch, as I commented above. I don't have any problem having 40 classes in a directory, at all. I just suggested (not "decided", I think I said "for your consideration" and that has a meaning, at least in Spanish) a viable alternative bringing one extra level of optional organization to developers worried about proliferation of classes in a single dir. Ciao Edited, to show the difference between decide and suggest.
            Hide
            timhunt Tim Hunt added a comment -

            I think the key point is c). Whether this level 3 proposal works well, or badly, depends on the typical class names for elements in the new Element library/renderer proposal. If the first word in the class name ends up being pretty arbitrary, then we will have a mess. If it ends up being something systematic like atom_..., molecule_... or organism_..., then the grouping will be logical. So, I would not integrate this until we know what the new recommended naming convention will be.

            Eloy, You just suggested "for our consideration" and then next minute Damyon had amended his patch to implement it with no consideration that was explained. That is what seemed odd to me.

            Regarding b), I will just say that if you learn something, it is nice if you can then share it in a comment.

            Also, note that using level3 was already suggested and rejected before. See, for example https://tracker.moodle.org/browse/MDL-41663?focusedCommentId=288994&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-288994, https://tracker.moodle.org/browse/MDL-41663?focusedCommentId=290978&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-290978

            The difference is that those previous suggestions were for using meaningful grouping at level 3, not just 'first word of the class name'. Perhaps it is worth re-condsidering those logical level 3 suggestions, in light of how the element library proposal is evolving?

            Show
            timhunt Tim Hunt added a comment - I think the key point is c). Whether this level 3 proposal works well, or badly, depends on the typical class names for elements in the new Element library/renderer proposal. If the first word in the class name ends up being pretty arbitrary, then we will have a mess. If it ends up being something systematic like atom_..., molecule_... or organism_..., then the grouping will be logical. So, I would not integrate this until we know what the new recommended naming convention will be. Eloy, You just suggested "for our consideration" and then next minute Damyon had amended his patch to implement it with no consideration that was explained. That is what seemed odd to me. Regarding b), I will just say that if you learn something, it is nice if you can then share it in a comment. Also, note that using level3 was already suggested and rejected before. See, for example https://tracker.moodle.org/browse/MDL-41663?focusedCommentId=288994&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-288994 , https://tracker.moodle.org/browse/MDL-41663?focusedCommentId=290978&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-290978 The difference is that those previous suggestions were for using meaningful grouping at level 3, not just 'first word of the class name'. Perhaps it is worth re-condsidering those logical level 3 suggestions, in light of how the element library proposal is evolving?
            Hide
            damyon Damyon Wiese added a comment -

            The difference between this and the previous suggestions for using the level 3 namespaces - is that this is entirely optional, and flexible - while the previous suggestions were mandatory and rigid.

            Show
            damyon Damyon Wiese added a comment - The difference between this and the previous suggestions for using the level 3 namespaces - is that this is entirely optional, and flexible - while the previous suggestions were mandatory and rigid.
            Hide
            timhunt Tim Hunt added a comment -

            Some fexibility is good. E.g. the idea to allow completely flat (everything directly in classes folder) for small plugins where that makes sense. However, the whole point of coding guidelines is to eliminate unnecessary flexibilty, which might also be called inconsistency. E.g. $camelCaseVariableNames. Not sure which side of the grey area the thing we are discussing here falls.

            Show
            timhunt Tim Hunt added a comment - Some fexibility is good. E.g. the idea to allow completely flat (everything directly in classes folder) for small plugins where that makes sense. However, the whole point of coding guidelines is to eliminate unnecessary flexibilty, which might also be called inconsistency. E.g. $camelCaseVariableNames. Not sure which side of the grey area the thing we are discussing here falls.
            Hide
            cibot CiBoT added a comment -

            Moving this issue to current integration cycle, will be reviewed soon. Thanks for the hard work!

            Show
            cibot CiBoT added a comment - Moving this issue to current integration cycle, will be reviewed soon. Thanks for the hard work!
            Hide
            poltawski Dan Poltawski added a comment -

            Integrated to master, thanks Damyon.

            I also like the current approach and there are enough opinions on this issue who are with this approach rather than Tim's, so sorry Tim.

            Show
            poltawski Dan Poltawski added a comment - Integrated to master, thanks Damyon. I also like the current approach and there are enough opinions on this issue who are with this approach rather than Tim's, so sorry Tim.
            Hide
            markn Mark Nelson added a comment - - edited

            Hey guys,

            Sam identified this as the issue that is causing the following error when viewing report/log/index.php

            Coding error detected, it must be fixed by a programmer: Can not render widget, renderer method (render_report_log) not found.
             
            Stack trace:
             
            line 109 of /lib/outputrenderers.php: coding_exception thrown
            line 236 of /lib/outputrenderers.php: call to renderer_base->render()
            line 182 of /report/log/index.php: call to plugin_renderer_base->render()
            

            Show
            markn Mark Nelson added a comment - - edited Hey guys, Sam identified this as the issue that is causing the following error when viewing report/log/index.php Coding error detected, it must be fixed by a programmer: Can not render widget, renderer method (render_report_log) not found.   Stack trace:   line 109 of /lib/outputrenderers.php: coding_exception thrown line 236 of /lib/outputrenderers.php: call to renderer_base->render() line 182 of /report/log/index.php: call to plugin_renderer_base->render()
            Hide
            marina Marina Glancy added a comment -

            Just to add to Mark's comment above, this can only be reproduced if you have only one logstore enabled

            Show
            marina Marina Glancy added a comment - Just to add to Mark's comment above, this can only be reproduced if you have only one logstore enabled
            Hide
            samhemelryk Sam Hemelryk added a comment -

            Ok I've had a think about this and have a solution - but it requires a little explaining perhaps.

            The problem is that this fix introduces a small but sensible change to how render methods are found. It removes the `_renderable` suffix if present from the class name when finding the render method for a renderable class.
            However this introduces a regression, as Mark found, whereby any existing renderables named like blah_renderable and with accompanying render method render_blah_renderable now break as we no longer look for render_blah_renderable and instead look for render_blah.

            The patch I purpose for this adds a debugging case to plugin_renderer_base so that if the render method is not found it will look for the deprecated method before passing off to the render chain.
            This essentially fixes the issue in all plugins affects, such as the log reports.
            The core_renderer/renderer_base don't need to be fixed because I have converted the three situations in core that are affected by this, and as there can't be more there is no need to handle it there (it would be redundant confusing code).
            The big downside to this is that it potentially introduces a regression in custom themes that have already overridden the render method for these reports, or any plugin affected components because this change breaks the render chain in these situations.

            Anyway have a look at the code and no doubt you'll understand.
            Also worth noting I took this opportunity seeing as I had to create renamed methods in the log reports to also correct the scope of the render methods there. Yay!

            Please have a look and a think, I will help out as I can.

            Cheers
            Sam

            Show
            samhemelryk Sam Hemelryk added a comment - Ok I've had a think about this and have a solution - but it requires a little explaining perhaps. Diff http://mdl.icodedthat.com/41663-28~1 Pull git pull git://github.com/samhemelryk/moodle.git 41663-28 The problem is that this fix introduces a small but sensible change to how render methods are found. It removes the `_renderable` suffix if present from the class name when finding the render method for a renderable class. However this introduces a regression, as Mark found, whereby any existing renderables named like blah_renderable and with accompanying render method render_blah_renderable now break as we no longer look for render_blah_renderable and instead look for render_blah . The patch I purpose for this adds a debugging case to plugin_renderer_base so that if the render method is not found it will look for the deprecated method before passing off to the render chain. This essentially fixes the issue in all plugins affects, such as the log reports. The core_renderer/renderer_base don't need to be fixed because I have converted the three situations in core that are affected by this, and as there can't be more there is no need to handle it there (it would be redundant confusing code). The big downside to this is that it potentially introduces a regression in custom themes that have already overridden the render method for these reports, or any plugin affected components because this change breaks the render chain in these situations. Anyway have a look at the code and no doubt you'll understand. Also worth noting I took this opportunity seeing as I had to create renamed methods in the log reports to also correct the scope of the render methods there. Yay! Please have a look and a think, I will help out as I can. Cheers Sam
            Hide
            marina Marina Glancy added a comment -

            Sam, I like your patch, thanks!

            Show
            marina Marina Glancy added a comment - Sam, I like your patch, thanks!
            Hide
            samhemelryk Sam Hemelryk added a comment -

            Thanks for the feedback on it Marina (we discussed it on integration chat and I made several changes for things Marina picked)

            Show
            samhemelryk Sam Hemelryk added a comment - Thanks for the feedback on it Marina (we discussed it on integration chat and I made several changes for things Marina picked)
            Hide
            marina Marina Glancy added a comment -

            Sam, fix integrated, thanks!

            Show
            marina Marina Glancy added a comment - Sam, fix integrated, thanks!
            Hide
            marina Marina Glancy added a comment -

            Tested almost everything during integration of the fix, behat is still running, will close the test when it's completed.

            Show
            marina Marina Glancy added a comment - Tested almost everything during integration of the fix, behat is still running, will close the test when it's completed.
            Hide
            marina Marina Glancy added a comment -

            cool, all tests passed. Thanks!

            Show
            marina Marina Glancy added a comment - cool, all tests passed. Thanks!
            Hide
            poltawski Dan Poltawski added a comment -

            This change is now part of Moodle! Thanks for your contribution!

            Before software can be reusable it first has to be usable.
            --Ralph Johnson

            Show
            poltawski Dan Poltawski added a comment - This change is now part of Moodle! Thanks for your contribution! Before software can be reusable it first has to be usable. --Ralph Johnson

              People

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

                Dates

                • Created:
                  Updated:
                  Resolved:
                  Fix Release Date:
                  10/Nov/14