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

Setup embedded tracker environment correctly in queuedtracking:process #17

Merged
merged 3 commits into from
Jul 24, 2015

Conversation

diosmosis
Copy link
Member

Use Environment instance instead of trying to clear the container and reload tracker plugins / environment in-place.

Should fix #12

@tsteur Can you test these changes? I'm not currently setup to test/debug QueuedTracking and there aren't any automated tests for this command. Feel free to commit directly to this branch, you'd know better how to apply this change to this plugin.

…using Environment instance instead of trying to clear the container.
@@ -37,13 +35,15 @@ protected function execute(InputInterface $input, OutputInterface $output)
$systemCheck = new SystemCheck();
$systemCheck->checkRedisIsInstalled();

Access::getInstance()->setSuperUserAccess(false);
Plugin\Manager::getInstance()->setTrackerPluginsNotToLoad(array('Provider'));
$trackerEnvironment = new Environment('tracker');
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will this recreate the container and clear any previous created instances? Especially re the Cache? I'm not into the environment / container code so just asking :)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it doesn't recreate the container, it creates a new one w/ the tracker environment. For now, this pushes a container instance onto a stack so StaticContainer::get() will use the top. Which I think means I missed something, there should probably be a $trackerEnvironment->destroy() call after processing is done, so control goes back to the CLI environment.

Note: This means there's a new Plugin\Manager and should be new Plugin instances stored in $trackerEnvironment. Since it's only done once here, shouldn't be an issue.

Eventually, we'll get rid of StaticContainer so everything is provided through DI, then we just use the environment directly and worry about nothing.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like this PR will fix a couple of issues.

Which I think means I missed something, there should probably be a $trackerEnvironment->destroy() call after processing is done, so control goes back to the CLI environment.

Does this mean there's more work left?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should be done w/ my last commit.

@ldidry
Copy link

ldidry commented Jul 23, 2015

I tested the patch. It's still not working. I got the following error:

PHP Fatal error:  Call to undefined method Piwik\Plugins\QueuedTracking\Queue\Manager::shouldRecordStatistics() in /var/www/piwik/plugins/QueuedTracking/Queue/Processor.php on line 68

@diosmosis
Copy link
Member Author

@ldidry fixed that error in my last commit.

@ldidry
Copy link

ldidry commented Jul 24, 2015

This error is fixed, indeed. But there is another problem: I got 534455 requests to process (said queuedtracking:monitor) but when I launch the process command, it says:

  This worker finished queue processing with 0req/s (0 requests in 0.00 seconds) 

tsteur added a commit that referenced this pull request Jul 24, 2015
Setup embedded tracker environment correctly in queuedtracking:process
@tsteur tsteur merged commit 4cc91ff into master Jul 24, 2015
@tsteur tsteur deleted the 12_process_env branch July 24, 2015 12:20
@tsteur
Copy link
Member

tsteur commented Jul 24, 2015

Thx @diosmosis It looks like it works now, tested as well. Also seems to use the right environement/cache (which is important for performance etc)

@diosmosis
Copy link
Member Author

@tsteur Would you know why @ldidry might be getting the error he mentioned? Do you think it might have something to do w/ DI (ie, wrong instance being used somewhere), or something else?

@tsteur
Copy link
Member

tsteur commented Jul 24, 2015

It worked for me. @ldidry are you still getting this error with latest version (0.1.7)? Can you maybe update to Piwik 2.14.2?

@ldidry
Copy link

ldidry commented Jul 25, 2015

I just upgraded both piwik and plugin to 2.14.2 and 0.1.7 this morning and this is working. Thanks a lot! 🍻

@tsteur
Copy link
Member

tsteur commented Jul 25, 2015

Thx! I haven't tested with Piwik 2.14.0 so I'm not sure whether plugin will work with that version. I'll release a new version 0.1.8 that requires Piwik 2.14.2 just to make sure it'll work

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

Successfully merging this pull request may close these issues.

Command line processing causing exception
3 participants