mdjnelson / moodle-mod_customcert

Enables the creation of dynamically generated certificates with complete customisation via the web browser.
https://moodle.org/plugins/mod_customcert
GNU General Public License v3.0
91 stars 158 forks source link

PHP notice running scheduled task to email students #443

Closed mdjnelson closed 5 months ago

mdjnelson commented 3 years ago
PHP Notice:  Coding problem: unsupported modification of PAGE->context from 70 to 70* line 1064 of /lib/pagelib.php: call to debugging()
* line 88 of /mod/customcert/classes/task/email_certificate_task.php: call to moodle_page->set_context()
* line 248 of /lib/cronlib.php: call to mod_customcert\task\email_certificate_task->execute()
* line 120 of /lib/cronlib.php: call to cron_run_inner_scheduled_task()
* line 73 of /lib/cronlib.php: call to cron_run_scheduled_tasks()
* line 79 of /admin/cli/cron.php: call to cron_run()
 in /var/www/vhosts/XXX.com/httpdocs/lib/weblib.php on line 3249
DataTriangle commented 2 years ago

Not a programmer, just server admin. I wanted to update this issue. I seem to not be getting successful runs of the email certificate task for the same context error.

Moodle [3.11.6+ (Build: 20220405)] Custom Certifcate: 3.11.1 (2021051701)

/opt/cpanel/ea-php74/root/usr/bin/php scheduled_task.php --execute='\mod_customcert\task\email_certificate_task' --showdebugging

Execute scheduled task: Handles emailing certificates. (mod_customcert\task\email_certificate_task) ... started 12:37:37. Current memory use 13.1MB.

++ Coding problem: unsupported modification of PAGE->context from 70 to 70 ++

TopSolid commented 1 year ago

Is there any workaround to this problem? Executing the cron in http could be one as according to this thread? https://github.com/mdjnelson/moodle-mod_customcert/issues/501 "Using the global $PAGE in a scheduled task only works when you execute the task inside the browser. During cron, there is no $PAGE available."

mdjnelson commented 1 year ago

I am unable to replicate this issue no matter how hard I try. Is this still an issue in 4.x? I am looking at using @ewallah patch at https://github.com/ewallah/moodle-mod_customcert/commit/d89d8ebf637de044e742c460d300a5cd870a43f9 but would rather be able to replicate this first.

I am running the scheduled task and an email is being sent but I don't get any PHP notice.

root@8c537884e9b6:/var/www/html# php admin/cli/scheduled_task.php --execute="\mod_customcert\task\email_certificate_task" --showdebugging
Execute scheduled task: Handles emailing certificates. (mod_customcert\task\email_certificate_task)
... started 07:50:17. Current memory use 12.7 MB.
... used 37 dbqueries
... used 0.43553614616394 seconds
Scheduled task complete: Handles emailing certificates. (mod_customcert\task\email_certificate_task)
root@8c537884e9b6:/var/www/html# 
mdjnelson commented 1 year ago

I see it is an issue in https://moodle.org/mod/forum/discuss.php?d=447004, so will use this patch. No idea why I can't replicate it.

mdjnelson commented 1 year ago

Reopening as I don't think the current solution is correct because we are just setting a random $page object's context, not the one that will ensure the course settings, such as language, are kept and don't default to the site settings. I need to be able to reproduce this and come up with a proper fix. Ill revert this later.

scyrma commented 1 year ago

Hi Mark, this is definitely still an issue on 4.2.1. I'm happy to help test potential fixes. I'll talk with @dustinmoodle, he might be able to help debug this on our side.

mdjnelson commented 1 year ago

Thanks @scyrma, that would be amazing and much appreciated! The solution I cherry-picked is at https://github.com/mdjnelson/moodle-mod_customcert/commit/9a11c537fbce90f02fa4bf8b3d0830589ae99d23, but I think it's wrong (see above comment by me) so I haven't released a new version yet on the plugins DB with this fix.

DataTriangle commented 1 year ago

I just saw the updates here. I am on customcert version: 3.11.3 2021051704

Moodle version 3.11.12 (We are waiting on some issue with 4 to be resolved that effect us upgrading. Hoping 4.3 will be a go for us.)

Anyways, I just tested. I still get the same error. Thanks for looking at this.

[root@host cli]# php scheduled_task.php --execute='\mod_customcert\task\email_certificate_task' --showdebugging Execute scheduled task: Handles emailing certificates. (mod_customcert\task\email_certificate_task) ... started 07:09:14. Current memory use 13.2MB. ++ Coding problem: unsupported modification of PAGE->context from 70 to 70 ++

DataTriangle commented 1 year ago

Just to be clear, that was with 9a11c53 manually applied.

ewallah commented 1 year ago

@DataTriangle

How can the system throws a coding problem error containing PAGE if the PAGE was replaced with page?

ewallah commented 1 year ago

'I am unable to replicate this issue no matter how hard I try.'

Have you tried cron? If I run the task inside the browser, I do not get an error, because in a browser the global $PAGE is always available. Nor when I run the cron task in a browser. But when cron is running in the background as a client script, the errors pop up in the cron log of the operating system (at least when you capture them).

In cron, there is no global $USER available and the global $PAGE is only available for backwards compatibility and should not be modified (see MDL-19774). Perhaps this is because text filters already can have modified page titles.

To solve the language issue, perhaps you could have a look at the function setup_user in the lib/classes/cron.php fille. There they ignore the timezone - language - locale preferences, but use the site default instead. So writing your own setup_user cron function perhaps could do the trick? Before Moodle decides after almost 13 years to delete this setup_user function.

danowar2k commented 8 months ago

It seems we're experiencing this too (Moodle 4.1.6, mod_customcert v4.0.3, in the scheduled task):

PHP Notice:  Coding problem: unsupported modification of PAGE->context from 70 to 70* line 1203 of /lib/pagelib.php: call to debugging()
* line 90 of /mod/customcert/classes/task/email_certificate_task.php: call to moodle_page->set_context()
* line 263 of /lib/cronlib.php: call to mod_customcert\task\email_certificate_task->execute()
* line 120 of /lib/cronlib.php: call to cron_run_inner_scheduled_task()
* line 73 of /lib/cronlib.php: call to cron_run_scheduled_tasks()
* line 178 of /admin/cli/cron.php: call to cron_run()  in /var/moodle/htdocs/moodle/lib/weblib.php on line 3351
danowar2k commented 8 months ago

I mtrace'd the contexts in the scheduled task. The above happens because you set the $PAGE->context each time for each certificate (inside a foreach). If Moodle's page context is system or course level, Moodle doesn't nag the developer. But once a module context has been set, Moodle doesn't want the context to be overridden again.

See

    public function set_context($context) {
        if ($context === null) {
            // Extremely ugly hack which sets context to some value in order to prevent warnings,
            // use only for core error handling!!!!
            if (!$this->_context) {
                $this->_context = context_system::instance();
            }
            return;
        }
        // Ideally we should set context only once.
        if (isset($this->_context) && $context->id !== $this->_context->id) {
            $current = $this->_context->contextlevel;
            if ($current == CONTEXT_SYSTEM or $current == CONTEXT_COURSE) {
                // Hmm - not ideal, but it might produce too many warnings due to the design of require_login.
            } else if ($current == CONTEXT_MODULE and ($parentcontext = $context->get_parent_context()) and
                $this->_context->id == $parentcontext->id) {
                // Hmm - most probably somebody did require_login() and after that set the block context.
            } else {
                // We do not want devs to do weird switching of context levels on the fly because we might have used
                // the context already such as in text filter in page title.
                debugging("Coding problem: unsupported modification of PAGE->context from {$current} to {$context->contextlevel}");
            }
        }

        $this->_context = $context;
    }

If you want to explicitly use the $PAGE/context's language settings etc. you'll have to change the task so that it only processes a single certificate each time it is run.

danowar2k commented 8 months ago

To replicate the output:

The debugging output won't be displayed when running the task via GUI.

I'm unsure why you really need to use $PAGE->set_context at this point? Can you show me a scenario where omitting that line leads to undesired results? If a custom certificate is explicitly set to be output in "Latin", which part of it would then be still output in the site language in this task?

adamjenkins commented 8 months ago

I was unable to replicate this issue on a clean install (as I mentioned in #588 ) however, the problem is persisting on a fully up-to-date populated instance I worked on. The clean install doesn't meet the criteria of having "multiple certificates which are sent by email" as mentioned by @danowar2k and I'm guessing this is the reason I couldn't replicate the error messages.

FWIW, I got my debug messages by running: admin/cli/scheduled_task.php --showdebugging --execute='\mod_customcert\task\email_certificate_task'

Appreciate the work on this and would be happy to test and report back again after the next release is released.

JackAidley commented 6 months ago

Hello @mdjnelson,

In my testing I think the problem can be fixed by moving the lines

        $page = new \moodle_page();
        $htmlrenderer = $page->get_renderer('mod_customcert', 'email', 'htmlemail');
        $textrenderer = $page->get_renderer('mod_customcert', 'email', 'textemail');

from between the comment The renderers used for sending emails. and the foreach statement to between the line fetching the comment $context = \context::instance_by_id($customcert->contextid); and the line applying the context to the page $page->set_context($context);. This means that it creates a new page each time through the loop, which I assume is inefficient to some degree, so I guess ideally it would check whether the contextid is changing and only create a new page with the new contextid when it changes?

As far as I can tell this doesn't cause any knock on problems, but I'm not at all familiar with this code so I wouldn't like to swear to that.

All the best and thank you for your hard work supporting the module,

Jack.

ewallah commented 6 months ago

I can confirm that

mdjnelson commented 6 months ago

Yep, I didn't close the ticket cause I knew it still existed.

Ill work on this one tomorrow all going well. Thanks to you both.

mdjnelson commented 5 months ago

Going to push a fix soon. I am just going to remove the call to set_context(). I believed this was needed to keep the language depending on various settings (force language course setting, users preferred language, site default etc etc) but it doesnt seem to be needed as I tested this in depth this afternoon, so in the end the fix is super simple and this should have been done ages ago. I apologise.