All yours Andrew

I think I have a workable solution.

https://github.com/andyjdavis/moodle/commit/fccde4fb076fb2d3848407d735f3787181173435

feedback welcome. If you have suggestions on how to better implement this let me know

Ho Andrew, some quick comments:

1) I think that search for functions should happen only for plugin pages.
2) And plugin_pagetypelist() does basically that. Surely you only need to add fallback to parent dir there.
3) IMO the problem is that the "if (array_key_exists($component, $plugins) " condition is completely wrong (only works for 1st level plugins, like mod/blocks, but not for the rest) and that isn't ever executed (for other plugin types. Also "$component" is messed there and does not point to proper "plugintype_pluginame_" style.

Hope it helps ciao

moved strings out of the core string files. https://github.com/andyjdavis/moodle/commit/20fc347172c3ff31fd5309b284be81f39eda6f4c

Another issue. generate_page_type_patterns() isn't generating valid function names under all circumstances. If a callback for the outline course report were written it should be called coursereport_outline_pagetypelist(). Not coursereportoutline_pagetypelist().

Some code-review on https://github.com/andyjdavis/moodle/commit/fccde4fb076fb2d3848407d735f3787181173435

1. coursereport_pagetypelist does not follow the coding guidelines. Should be _page_type_list
2. First array in that function is not properly laid out: http://docs.moodle.org/dev/Coding_style#Wrapping_Arrays or http://docs.moodle.org/dev/Coding_style#Associative_arrays
3. if (count($bits >= 3)) { is wrong
4. for($i should be for ($i
5. implode($componentarray) - I had to look this up to confirm it was valid. I would always spell this out as implode('', $componentarray)
6. Anyway, I am not sure that $component = implode($componentarray) works in any case other than coursereport. What about report -> admin/report; qtype -> question/type, ...

Now looking at https://github.com/andyjdavis/moodle/commit/20fc347172c3ff31fd5309b284be81f39eda6f4c

1. I am not sure that 'Any log course report' etc. make sense. There is only one log report.
2. It is a bit sucky to change the plugin API to require all course reports to add this string, for such a minor feature as this. Is it really necessary?

As per dev chat:

(10:57:08 AM) Eloy: andrew have you read my comment? it seems that plugin_pagetypelist() already does a big part of what you're trying to achieve. The only problem is that the:

"if (array_key_exists($component, $plugins)"

condition only works for 1st level plugins! (mod, blocks...) but not for the rest!
(10:59:15 AM) Eloy: and of course "$component" is also wrong. Must be, always, plugintype_pluginame
(10:59:22 AM) Eloy: cya
(10:59:29 AM) timhunt: Yes. We need a reliable way to get the frankenstyle name of the plugin responsible for this page from $PAGE. I think we need a specific $PAGE->get_owning_component() method.
(10:59:39 AM) timhunt: This logic should not be mixed up in generate_page_type_patterns
(10:59:43 AM) Eloy: yup

Ive started fixing this stuff. Branch is MDL-27829_block_page_type_fixes Just recording this here so I know what branch to pull when I get home.

Ok. Ive heavily reworked this. Far fewer lib files will be loaded although the page type will be pulled apart twice. Probably more work to do there.

Note that this isnt finished code. Theres still commented out code lying around in this plus I havent tested this particularly thoroughly. Just after some general feedback from those in the know.

Most of the action is in generate_page_type_patterns().

Im currently intending on creating functions for each course report ie outline_page_type_list() for coursereport_outline. coursereport_page_type_list() will disappear. That means more functions but the functions will be really simple pieces of code. I figured I was trying to be a bit fancy and possibly creating problems for the future trying to do this dynamically.

Ive created the various course report callbacks. Ive also been able to remove the need to iterate through the page type more than once. I think this is more or less ready. Peer review requested.

Hi Andrew,

I've just been looking over this now.
In regards to the patch I noticed only a couple of minor things that don't change functionality but really just help the readability of the generate_page_type_patterns function.
*lib/blocklib.php

• • ln 1593: Is there any need to the array_slice, surely $pluginname = $bits[$i] is fine?
  • ln 1582: On this line you check whether the result of the function call is empty but on lines 1607 and 1617 you don't.
  • ln 1613: $patterns will always be empty
  • ln 1626: Is there any need for this last !empty check of $patterns, we know that default_page_type_list will always return at least the $patterns array being built within that if.

Other than that I think the patch is looking good.

There is one thing worth nothing however and that is that I REALLY dislike the function names being used by this function + callbacks.
They were function names I think I used when I first wrote the bit of code to make use of core component functions and I really regret the way I've named them.
It doesn't really have any bearing on this issue, I will create an issue shortly when I have time to gather my thoughts about it.

Cheers
Sam

Just found a warning when adding a block to the course report listing screen:

Notice: Undefined offset: 0 in /var/git/peer-review/master/lib/blocklib.php on line 1594
Call Stack

1. Time Memory Function Location
   1 0.0003 662080
   Unknown macro: {main}

   ( ) ../report.php:0
   2 0.0795 24029992 bootstrap_renderer->header( ) ../report.php:20
   3 0.0795 24030384 bootstrap_renderer->__call( ) ../setuplib.php:0
   4 0.0812 24116744 call_user_func_array ( ) ../setuplib.php:1245
   5 0.0812 24117112 core_renderer->header( ) ../setuplib.php:0
   6 0.0812 24117192 moodle_page->set_state( ) ../outputrenderers.php:607
   7 0.0812 24117192 moodle_page->starting_output( ) ../pagelib.php:718
   8 0.0817 24139272 block_manager->process_url_actions( ) ../pagelib.php:1206
   9 0.0818 24139272 block_manager->process_url_edit( ) ../blocklib.php:1047
   10 0.1201 34184536 block_edit_form->__construct( ) ../blocklib.php:1190
   11 0.1201 34184536 moodleform->moodleform( ) ../edit_form.php:59
   12 0.1208 34209736 block_edit_form->definition( ) ../formslib.php:152
   13 0.1441 37525720 generate_page_type_patterns( ) ../edit_form.php:132

Ok. I think thats all done.

Hi Eloy,

I've had a look at this and it get's my +1.
I've made you the integrator as you've done the previous reviews and helped out with it etc...
Let me know if you don't have time and I will give it a final once over before putting it in myself.

Cheerio
Sam

Hi Sam, I'm moving this back to you as far as I assume it's fresher in your mind. So I will, for sure, be able to do other 5x integrations with the same effort.

If at the end of the day I've time to take a look to this I will (although I'm 99% sure I won't). Thanks for all the effort Andrew!

Ciao

This has been integrated now thanks Andrew.

Tried logging in as a teacher, adding a block to a course page and then configuring the block. I obtained the following options:

• Any type of course main page
• Any page
• Any course page
• Any activity module page

The option 'Any activity module page' wasn't available previously. It doesn't seem a sensible option to me. If selected, the block doesn't appear on the course page but instead appears on every activity module page i.e. a lot of work to remove if added by accident! Thus I would recommend that this new option is removed.

Tried adding a block to a quiz activity and noticed the setting 'Display on page types' is missing. It is available when adding a block to an assignment activity and is also available when adding a block to a quiz activity in 2.0.3+.

Sorry I'm failing this issue as, after following the testing instructions 'check that the "display on page types" drop down containing a sensible set of options', I have found that the options are not sensible or in the case of quiz activities, missing entirely.

Many thanks, Helen!

A) Well, there are 2 (alternative) ways here:

1) The test instructions were way too general for this issue. So then, they must be better specified, so this can be retested and passed.
2) The test instructions are correct and the problems reported by Helen are related to this, so we must revert this or fix in in the next 16 hours.

B) And there is also one conclusion: The test: "check that the "display on" dropdown won't pass, no matter which issue is the responsible (I bet it was MDL-26105 the one introducing the problem). But the fact is that the problem exists. One again, there are two alternatives:

1) Move the problems to recently created MDL-27829 and ignore.
2) Fix the wrong behaviors in the next 16h.

So, guys, you've to decide about both A) and B) ASAP!

Ciao

PS: Moodle 2.2 will stop accepting improvements and new features at least 1 month before release. All these issues open hours before release cannot happen again IMO (just a reflexion).

Eloy, you are correct. This world of hurt was introduced by MDL-26105. I may print out a big banner that says "Thou shalt not rush through big changes just before release". It should probably go over my desk

I was going to do further bug fixing in MDL-27893 but instead I think I will close that issue and do it against this one.

Comment by me in MDL-27893:

Here are the various page types you can in the various areas. Ive added notes describing those that were present in 2.0.3 which don't exist in 2.1

front page:

site-index
site-index-*
*

(perfect match)

course:

course-*       any course page
course-view-*  any type of course main page
mod-*          any activity module page
*              any page

2 additional options were available in 2.0.3

course-view-weeks   any course main page in weeks format
course-view-weeks-* ?

activity:
no options given. Need to check what page type is being used.
additional options were available in 2.0.3

mod-assignment-view
mod-*-view          any main activity module page
mod-assignment-view-*
mod-assignment-*
mod-*               any activity module page
*                   any page

course report (outline report used as example):

course-report-outline-*  course activity report
course-report-*          any course report
*                        any page

Additional options were available in 2.0.3

course-report-outline-index
course-report-outline-index-*
course-*

admin page:

front page only
front page and any pages added to the front page
entire site

This is clearly wrong. Here are the 2.0.3 options

admin-setting-optionalsubsystems
admin-setting-optionalsubsystems-*
admin-setting-*
admin-*
*

my moodle
no options given. Need to check what page type is being used.
additional options were available in 2.0.3

my-index
my-index-*
my-*
*

"The option 'Any activity module page' wasn't available previously. It doesn't seem a sensible option to me. If selected, the block doesn't appear on the course page but instead appears on every activity module page i.e. a lot of work to remove if added by accident! Thus I would recommend that this new option is removed."

A block added to "any activity module page" doesnt appear on the course page but does appear on all module pages. Its still a single block so if you delete it from any of those activities its deleted from all of them. All the same, it is a little weird so unless theres real demand for it (there isnt afaik) its gone.

I spoke to Dongsheng about this issue. He was the original author of much of the code and I wasn't party to the original discussions about what was to be done. Apparently there was a conscious decision to discard some of the options as some of them were identical in practice.

If a block has been configured to use an option that has been removed it will continue to work. You just won't be able to add new blocks with the same page types setting. Editing a block set to a now removed page type could be a bit weird.

Adding a block to an activity, I used database for this example, causes the block to have its page type restriction set to mod-data-view. I suspect that the 2.0.3 set of options has been whittled down to a single option. As there is only a single option the drop down box is omitted from the block setting page.

Similarly, blocks added to the my moodle page automatically get the page type of my-index rather than the set of mostly pointless options. I believe this is by design.

SUMMARY: I believe are three things I have to do.

1) the set of page types presented for a block on a admin settings page is incorrect.

2) the testing instructions need to be improved.

3) add a block then manually edit the database to set its page type to be one of the removed page types. check that nothing bad happens.

#1 there is a new commit that fixes admin setting pages. https://github.com/andyjdavis/moodle/commit/800688bd1ef460c6447779d77780c16fc90348a7

#2 testing instructions updated.

#3 There was some code to deal with the removal of selectable page types however it was broken. Somewhat fixed now https://github.com/andyjdavis/moodle/commit/096928c79e4ed6f0145de4099b06cbeb6dbc9b62

There is a bit of an issue with dealing with blocks set to show on page types which we no longer allow the user to select. I added code to add the page type to the list of options but then you wind up with mod-data-view being added as it doesnt match mod-data-*

I then added some intelligence so that it realized that mod-data-view matches mod-data-* but then it stops adding course-view-weeks because it finds course-view-*.

Right now, it will add a new option if the block specifies a now non-selectable page type and it can find an appropriate string. Otherwise your block continues to work fine but when you open its settings the drop down box will just default to the first option.

Eloy, can you please have a look at the two commits in my last comment?

Helen, if Eloy's happy with them, once they're integrated this can be retested.

Oki, I guess you are talking about:

• 800688bd1ef460c6447779d77780c16fc90348a7 (admin)
• 096928c79e4ed6f0145de4099b06cbeb6dbc9b62 (the typo)
• d097ad564df6facad0e6cf8937b91610eeb61d62 (the display warning)

(the 3 last commits in the MDL-27829_block_pagetype_master branch)

They look perfect, I'm going to re-integrate them to ignite a new testing round. Let's see how it behaves now.

Thanks!

Failing as two out of the four items listed in the testing instructions were not as described:

1. on a course main page:
any page
any course page
any type of course page - PASS

2. on an activity within a course:
you shouldn't get a drop down box at all as this is handled automatically - FAIL

Adding a block to an assignment activity, the block's settings had a display on page types dropdown with the following options:

Assignment module main page
Any assignment module page
Assignment module submissions page

Whilst I'm all in favour of simplifying the UI where possible, I'm concerned that removing the display on page types dropdown completely is taking things too far. Certain modules e.g. assignment and quiz have a variety of different pages. In Moodle 2.0 the new blocks functionality enabled blocks to be added to certain activity module pages but not all. For example, quiz included the following options:

mod-quiz-view
mod-*-view
mod-quiz-*

This meant that a block could be added to mod/quiz/view.php but not mod/quiz/report.php for example.

I would recommend that we think carefully before removing functionality that people may be depending upon.

3. on a course report like the course activity report (Courses > course name > Reports > activity report):
any page
any course report
just the current course report ie "Course activity report" it should actually tell you the name of the report youre looking at) - FAIL

Adding a block to the activity completion report, the block's settings had a display on page types dropdown which included the option 'Progress course report' instead of 'Activity completion report'.

Also it stated 'The previously specified page type is no longer selectable. Please choose the most appropriate page type below.' No page type was specified previously, as the block was newly created!

I tried adding several different blocks to the activity completion report page, all with the same results.

4. on an admin setting page like Advanced features (Site administration > Advanced features):
The current admin setting page
Any admin setting page - PASS

About 2) Yes, really i think that the (previously existing) missing options in some activities like quiz, is going too far. It sounds to me that such feature was used also for deciding which pages in one attempt show a given block (note I'm not 100% sure about this, perhaps that is just a dream, lol). Also sounds reasonable to have access to all the subplugins available (quiz reports), like assignment types and so on.

About 3) yup, needs fixing too. Also the newly introduced "The previously specified page type is no longer ..." string... why is it showing there? One new block should not require any conversion from 2.0.x options, so shouldn't be shown. That escaped for sure to my review, apologizes.

So this really needs a certain degree of improvement. I think Helen has explained it perfectly in the previous comment, thanks!

Ciao

re #2, each module returns a set of page types. If theres only 1 page type no option is displayed to the user. If there is more than 1 the drop down is displayed. I guess I need to go through each activity module and figure out which had options that were removed and out of those which need to go back in.

#3 Ive fixed up the strings. It was saying 'Progress course report' instead of 'Activity completion report' because that report is called the progress report internally but its called the activity completion report in the UI and I was confused. Eloy, there is a new commit in the branch.
https://github.com/andyjdavis/moodle/commit/1fe0ecabf15f6543227be821fbe7c7d772755f43

The "The previously specified page type is no longer selectable. Please choose the most appropriate page type below." message appears to be being displayed sometimes when it shouldn't be. I'll figure out why.

2.0.3 assignment:
mod-assignment-view
mod-*-view
mod-assignment-view-*
mod-assignment-*
mod-*
*

2.1 assignment:
mod-assignment-*
mod-assignment-view
mod-assignment-submissions

I think that looks fairly reasonable. I think...

2.0.3 quiz:
mod-quiz-edit
mod-quiz-edit-*
mod-quiz-*
mod-*
*

2.1 quiz:
mod-quiz-*
mod-quiz-edit (I just added this)

2.0.3 glossary:
mod-glossary-view
mod-*-view
mod-glossary-view-*
mod-glossary-*
mod-*
*

2.1 glossary:
mod-glossary-*
mod-glossary-view (I just added this)
mod-glossary-edit (I just added this)

2.0.3 lesson:
mod-lesson-edit
mod-lesson-edit-*
mod-lesson-*
mod-*
*

2.1 lesson:
mod-lesson-*
mod-lesson-view (just added)
mod-lesson-edit (just added)
mod-lesson-report (could add)
mod-lesson-essay (could add)

Because lesson supports sub pages an additional drop down box appears containing "any page matching the above" and "this specific page". I do not pretend to entirely understand what this does.

Ive committed changes to add some more options (see the above posts). Also in that commit is changes to the course report page type list functions to prevent the incorrect display of the missing page type message.

So Eloy, there are two new commits now.

1) course report strings.
https://github.com/andyjdavis/moodle/commit/1fe0ecabf15f6543227be821fbe7c7d772755f43

2) added page types that I think we want.
https://github.com/andyjdavis/moodle/commit/346a32a75b4fc1b0aa51355424d9d41a0c251fa2

I'm going to continue comparing activities between 2.0.3 and 2.1 so a third commit may follow.

2.0.3 forum:
mod-forum-view
mod-*-view
mod-forum-view-*
mod-forum-*
mod-*
*

2.1 forum:
mod-forum-*
mod-forum-view
mod-forum-discuss

I think this looks ok.

2.0.3 wiki:
mod-wiki-view
mod-*-view
mod-wiki-view-*
mod-wiki-*
mod-*
*

2.1 wiki:
mod-wiki-*
mod-wiki-view
mod-wiki-comments
mod-wiki-history
mod-wiki-map

I think this looks ok.

Further improved testing instructions.

Added a third commit since the last round of integration and testing.

https://github.com/andyjdavis/moodle/commit/2f9bdadfd23321951a7fbf7a33ddc5af57c5ef74

I thought all site admin pages have page types like admin-setting-* however ones exist like admin-registration-index so if you want a block to display on all of them it needs to be admin-*

Re-re integrated, thanks Andrew!

crosses fingers

After one more iteration we have found some problems that will be alleviated / fixed by these commits:

1) e5a863e55c3dcb8067cb5ce4bcdc8689bbda45b3 - fixed one course report string
2) ddaa9147895528f9baa5d6ec568f49c4db7a7220 - on creation of blocks, ensure that the pagetypepattern inserted is a valid one (that makes the message in the form to disappear for just created pages). Note this is only being applied to /mod pages, so other pages like course-report will continue showing the message. MDL-28150 (and MDL-28149) have been created about to decide if the /mod change should be applied ASAP to all the rest of pages. (this patch was peer-reviewed by Tim).
3) 3e16a2ca66ff2e05125e22b78fe990d9ee479b44 - clean one dupe comment in code
4) 715a5bb732b6d8c5875464ffd3b80c4ece5c5b0a - some more lang strings

And that's all, about to integrate them in order to perform one more testing...

Ciao

Yes! Finally this test passes Many thanks to Andrew and Eloy.

Small problem of 'The previously specified page type is no longer selectable...' being displayed on a non mod page is mentioned in MDL-28150.

Big thanks to Andrew and Helen! This is going to be the most-iterated issue ever, yay.

I'm raising the 2 followups of this as blockers for next week: MDL-28149 and MDL-28150

Ciao

PS: I think we won't be backporting this to 20_STABLE, lol. Let's see.

And there was much rejoicing! Thanks to all on this!

Many thanks Andrew and Helen! This was hard! Yay!