dbnschools / moodle-theme_fordson

Theme for Moodle 3.3+
42 stars 40 forks source link

LTI Tool Provider fails because of overridden favicon method #26

Closed eSrem closed 5 years ago

eSrem commented 6 years ago

Hi there!

I just found out that in a Moodle 3.5 environment the LTI Tool Provider enrolment does not work when using Fordson. This is because Fordson overrides the default favicon() method.

Context: The enrolment method LTI Provider tries to "get_icon()" and then call the method out() on the returned object. The error received is "Exception - Call to a member function out() on null" when Fordson doesn't have a favicon configured, or null is replaced by string when the icon is configured.

The original favicon method in $CFG->dirroot . /lib/outputrenderers returns a moodle_url object.

4082     public function favicon() {
   1         return $this->image_url('favicon', 'theme');
   2     }

The overruled favicon method in theme_fordson/classes/output/core_renderer.php returns a string or null.

1440     public function favicon() {
   1         return $this->page->theme->setting_file_url('favicon', 'favicon');
   2     }

My hotfix is to not override the favicon method. The permafix would be to rework the favicon method so it returns a moodle_url object.

1440     public function favicon() {
   1         return new \moodle_url($this->page->theme->setting_file_url('favicon', 'favicon'));
   2     }

However this requires another change and I have not yet figured out where in the plug-in this would need to be done.

dbnschools commented 6 years ago

If you open up the Fordson core_renderer.php file and locate the favicon function can you change it to this:

public function favicon() { if (!empty($this->page->theme->setting_file_url('favicon', 'favicon'))) { return $this->page->theme->setting_file_url('favicon', 'favicon'); } else { return $this->image_url('favicon', 'theme'); } } That should make the function default to the Moodle core function if there is no image in the custom fordson setting.

dbnschools commented 6 years ago

public function favicon() { if (!empty($this->page->theme->setting_file_url('favicon', 'favicon'))) { return $this->page->theme->setting_file_url('favicon', 'favicon'); } else { return $this->image_url('favicon', 'theme'); } }

around line 1441 of core_renderer.php

dbnschools commented 6 years ago

Let me know if that fixes the issue.

eSrem commented 6 years ago

This is a workaround; however the problem with LTI will still occur when you load a favicon within the theme settings. This is because the method setting_file_url returns a string. The core favicon method that's being overridden returns a moodle_url and as such the core plug-ins expect a moodle_url to be returned. The LTI enrolment method does not work because it calls for a favicon and is given a string instead of an object.

dbnschools commented 6 years ago

Do you have an idea on how we can keep the custom favicon and still make this work?

eSrem commented 6 years ago

Sadly I do not immediately. We'd need for a way to get the output URL sent to the template. I'm going to plan in my agenda to see what we can do beyond overriding the favicon. I'll get back to you!

pkgh commented 6 years ago

I want to confirm this is causing issues with LTI connectivity... I have encountered the same issue when using a different theme - Boost Campus, which also permits the upload of a custom favicon. After switching to the plain Boost theme, the problem was gone.

dbnschools commented 6 years ago

We use LTI with Wordpress and have no issues. I'm not sure it is an issue.

pkgh commented 6 years ago

This could be a:

dbnschools commented 6 years ago

Thanks. If a solution is found please update this thread and I will implement whatever needs to be done.

gemguardian commented 6 years ago

Hello, This bug effects all theme's that have the favicon upload implemented. I discoved the issue as far as 3.3.

For Theme Essential we just fixed this issue. See the https://moodle.org/mod/forum/discuss.php?d=371252#p1511614 for Gareths input.

As well the fix in for 3.4 / 3.5 https://moodle.org/mod/forum/discuss.php?d=376177

Gemma

dbnschools commented 6 years ago

Hello Gemma, Can you open up core_renderer.php and replace the favicon function on line 1541 as follows:

public function favicon() {
        $favicon = $this->page->theme->setting_file_url('favicon', 'favicon');

        if (empty($favicon)) {
            return $this->page->theme->image_url('favicon', 'theme');
        } else {
            return $favicon;
        }
}

Let me know if this fixes the issue.

gytisc commented 4 years ago

@dbnschools @eSrem @gemguardian @pkgh Hi All, I want to reopen this issue as far as LTI publishing from Moodle with Fordson is still not working (or not working again). I tried changing to Boost for the testing purpose and it helped to confirm that this is a Fordson problem. Changing theme for a particular course doesn't helps, you need to change it for all site. I see latest suggestion from @dbnschools is already integrated into the Fordson, so it should be another issue or some new bug? Please help, we love Fordson and use it in many Moodle based projects so far but this is a serious stopper for future considerations.

gemguardian commented 4 years ago

Hi, Sorry I don't use Fordson anymore. But I suggest you add the Moodle version and Fordson theme version to this comment, as well have you tried to set debuggin on your site and see what error message appears.

This might give a clue.

Gemma

dbnschools commented 4 years ago

@gytisc I don't know what version you are using but I believe it is fixed using the code provided in this thread. What is your Moodle and Fordson versions?

gytisc commented 4 years ago

@dbnschools we use latest Moodle 3.8.1+ (Build: 20200124) and Fordson v3.8 release 1.2

ndahn commented 2 years ago

As @eSrem wrote, the favicon method needs to return a moodle_url. The solutions suggested by @dbnschools so far do not work. Changing https://github.com/dbnschools/moodle-theme_fordson/blob/ba04c6d4456f32dc2f49be52faefcbdf56d5f863/classes/output/core_renderer.php#L1795 to return new moodle_url($favicon); makes LTI work, but no icon will be shown anymore. I'm not sure where this is called from...

Since this is not resolved it would be a good idea to reopen!

ndahn commented 2 years ago

Since moodle is not willing to even provide a temporary fix it seems like you will have to get active sooner or later on this... https://tracker.moodle.org/browse/MDL-72659