Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Running local_analytics on Moodle 3.1 #2

Open
joelsrf opened this issue Aug 12, 2016 · 15 comments
Open

Running local_analytics on Moodle 3.1 #2

joelsrf opened this issue Aug 12, 2016 · 15 comments

Comments

@joelsrf
Copy link

joelsrf commented Aug 12, 2016

Hello everyone

Using Moodle 3.1 and Google Universal Analytics the script doesn't get implemented into the site. Meaning if I check the source code no google javascript is visible.

Has anyone the same issue or was able to resolve it? It worked perfectly fine running Moodle 2.7.

@bmbrands
Copy link
Owner

What theme are you using on your site? Perhaps the theme layout files do not include the standard Moodle footer hmtl

@lucaboesch
Copy link

Same here.
Bas, where in the code do you assure that local_analytics/index.php and therefore local_analytics/lib.php is called?

@joelsrf
Copy link
Author

joelsrf commented Aug 16, 2016

We have a customized version of the Clean theme, but I'm pretty sure that the default Moodle footer is included.

@bmbrands
Copy link
Owner

bmbrands commented Aug 16, 2016

I did some more research and looks like Moodle no longer auto include lib.php files from local plugins.
As a workaround you could override Moodle's core renderer for the standard_footer_html() function in your theme like this:

public function standard_footer_html() { global $CFG; require_once($CFG->dirroot . '/local/analytics/lib.php'); return parent::standard_footer_html(); }

Or you can add

<?php require_once($CFG->dirroot . '/local/analytics/lib.php'); ?>

In your theme layout file.

Hopefully I'll find some other way of auto including the lib.php again.

@lucaboesch
Copy link

lucaboesch commented Aug 16, 2016

Props to you, Bas.
Just to note overriding the standard_footer_html() function fixes the issue only when Tracking code location (local_analytics | location) is "Footer".
"Top of body" and "Header" would not work with this workaround.
Let me write the whole function properly, you might as well add a control structure to check whether the local_analytics plugin is indeed installed. Otherwise the require_once() call might throw an error.

class theme_xxxxxxx_core_renderer extends theme_bootstrapbase_core_renderer {

    /**
     * The standard tags (typically performance information and validation links,
     * if we are in developer debug mode) that should be output in the footer area
     * of the page. Designed to be called in theme layout.php files.
     *
     * @return string HTML fragment.
     */
    public function standard_footer_html() {
        global $CFG;
        if (core_plugin_manager::instance()->get_plugin_info('local_analytics')) {
            require_once($CFG->dirroot . '/local/analytics/lib.php');
        }
        return parent::standard_footer_html();
    }
}

@joelsrf
Copy link
Author

joelsrf commented Aug 17, 2016

Great, thanks for the fast response and your help! However I'll probably wait until there is a fix in the plugin itself! Thanks!

@erazorbg
Copy link

I noticed that when you purge the caches, the lib.php file is included and the plugin works for 1 page load.
After some digging I found this (moodle/moodle@a0ac16c) commit that I think unintentionally changed how Moodle loads the local plugin's lib.php file.

It seems the new behavior is looking for a function that extends navigation, so I added a dummy function which fixed the issue for me.

Here is what I added at the end of lib.php

function local_analytics_extend_navigation($param){

}

@brendanheywood
Copy link

FYI we've also fixed this locally using the _extend_navigation hook BUT this does not capture all the pages, notable it misses those that do not include navigation. This includes some pretty important pages including the login page, help pages, the new grading page in mod_assign, ...

The reality is we need a new set of proper hook for plugins to inject stuff like this into the head, see this related tracker (and vote for it):

https://tracker.moodle.org/browse/MDL-53978

@brendanheywood brendanheywood mentioned this issue Oct 10, 2016
@brendanheywood
Copy link

Hey just an FYI that https://tracker.moodle.org/browse/MDL-53978 has been submitted and now that the epic 'hooks' tracker has been closed as 'won't fix' there is a reasonable change that these new callbacks will be accepted into core shortly (fingers crossed)

We will provide a pr shortly which pre-preemptively implements these hooks for review, and we can merge that in as soon as the core patch hits.

@roperto
Copy link

roperto commented Jan 20, 2017

Hi. I published some fixes based on the dev branch (with Nigel's improvements). I made something based on @erazorbg 's idea. It seems to be working on Moodle 3.1 now.

I also implemented the callback as suggested by @brendanheywood .

If someone wants to test/provide some feedback, the code is here:
https://github.com/catalyst/moodle-local_analytics

@bmbrands I didn't make a pull request because it is based on your dev branch instead of master, if you want to go that direction let me know and I can make the PR.

Cheers,

Daniel

@brendanheywood
Copy link

@bmbrands just throwing the idea out there - would you consider us taking over either partial or full maintenance of this plugin? We have a deep vested interest in it and are actively supporting our fork which has solved all these issues long ago and supports every stable version from 2.7 to master

https://github.com/catalyst/moodle-local_analytics

@bmbrands
Copy link
Owner

bmbrands commented Mar 1, 2017

Hi @brendanheywood I really appreciate your work on this plugin, but I would prefer collaborating on development and maintenance. I have not been able to invest much time in this plugin lately but I am willing to look at all your improvements and implement them on this repo. And, of course, give you all the credits for your input.

@brendanheywood
Copy link

Yes our overwhelming preference is keep it all together and just submit PR's to this repo.

But .... that big pull request we submitted was merged (#3) and then rolled back (#4) six months ago and nothing has been done since. This repo in it's current state only supports 2.7 which is out of support in a month or so, and this plugin simply doesn't work out-of-the-box at all anymore in any of the last 4 major versions of moodle due to the callback changes in 2.9. version.php hasn't had a commit in 2 years and the last version added to the plugin directory was from 2014.

The reality is that for all intents and purposes this plugin has been functionally abandoned. I'm not trying to be pushy and take it over, it's actually a hassle for us taking on maintenance of yet another plugin. I'm just offering whatever I can to help you out here and if you don't have time then we are happy to step up.

If you aren't interested in the pull request then that's ok too. In that case we'll make a proper fork and rename it and submit it separately to the plugin directory.

@bmbrands
Copy link
Owner

bmbrands commented Mar 9, 2017

Hi @brendanheywood I am getting some client funding to work on this plugin, create a new version and look at the work you have put into it too.

I have reserved time this and next week to work on this plugin. Thanks for the wakeup call!

@brendanheywood
Copy link

great news @bmbrands :)

also as a heads up @dmitriim has been working on another fairly major refactor so that the same plugin can handle multiple instances of ga / gu / piwik side by side, each with potentially different dimensions and config. For example we routinely deal with clients who have their own GA account but we also push data into our own internally hosted piwik instance as well. This work is not yet complete but should be in a couple weeks. wip branch:

https://github.com/catalyst/moodle-local_analytics/tree/issue%233

diff from our stable fork:

catalyst/moodle-local_analytics@catalyst-master...catalyst:issue#3

I'm fairly conscious that we are quickly diverging even more from your branch, so let me know what we can do to get things into shape to be merged back in. The sooner we can get these repo's consistent again the better

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

6 participants