@robocoder opened this Pull Request on February 12th 2018 Contributor
@sgiehl commented on February 12th 2018 Member

@sprhawk @kiwi-x @hvcgi @matfax Is anybody of you able to prove this fix works?

@mattab commented on March 22nd 2018 Member

if anyone is using Azure for the Matomo Database, and experiencing this issue, then please try this patch, and let us know if it fixes the issue. As soon as someone confirms we will merge it, thanks.

@kiwi-x commented on March 22nd 2018

Hallo.

Sorry for late answer. I didn't recognized the comment on 12. Feb.

I just downloaded the both raw files and replaced them in my azure instance and
the problem isn't solved.

Sorry.

I will try to debug the both methods.

Thanks for your work so far.

Regards
Kiwi-X

@kiwi-x commented on March 22nd 2018

At the moment it looks like the Markeplace calls only getServerVersion from The Pdo/Mysql.php.

At the Moment I don't now what it will check, but I still get the error message "Invalid MySQL Version"

@sgiehl commented on March 22nd 2018 Member

Maybe the data is cached somewhere. You can try to clear the tmp directory of Matomo to clear the file caches... Or run a ./console cache:clear if you have ssh access

@kiwi-x commented on March 22nd 2018

Hi,
I cleared the cache, but no change.

At the moment it looks like that in core/Db/Adapter/Pdo/Mysql.php the $this._config['host'] isn't set, so it doesn't walk into this if statement:

if (isset($this->_config['host']) && strpos($this->_config['host'], '.mysql.database.azure.com') !== false) {

[UPDATE 1]
When using $this->_config['host'] I get the following error on dashboard:

WARNING: /var/www/htmls/p/rm/core/Db/Adapter/Pdo/Mysql.php(122): Notice - Undefined index: host - Matomo 3.3.0 - Please report this message in the Matomo forums: https://forum.matomo.org (please do a search first as it might have been reported already)

So it won't check for @@VERSION.

[UPDATE 2]

If i comment out the if statement and let it override anytime, the Marketplace will start. So checking $this->_config['host'] isn't the right thing

@robocoder commented on March 23rd 2018 Contributor

Sorry. To make this work, you need to comment out the $adapter->resetConfig(); from https://github.com/matomo-org/matomo/blob/67f78cec4c03dc9b3451a3d08d33e9ea2bec3d3f/core/Db/Adapter.php#L55

but that re-opens an information disclosure risk.

@kiwi-x commented on March 23rd 2018

Hello.

After changing Adapter.php too I can confirm that Marketplace is running on Azure as expected.

But I can't evaluate if the risk of information disclosure is worth to fix the Marketplace in Azure.

Thanks so far

Cheers
Kiwi-X

@robocoder commented on March 23rd 2018 Contributor

I just thought of a workaround. I'll implement later this evening.

@robocoder commented on March 24th 2018 Contributor

I added an isAzure property which limits the information disclosure risk.

@kiwi-x commented on March 26th 2018

Good morning.

It looks like that this change breaks matomo. I now install a second instance for testing purposes only.

I will give you a second feedback as sonn I was able to test against second instance.

[UPDATE]
After a new installation and the replacement of the three files in this commit - I get the following error message:

Matomo encoutered an error: Call to undefined method Piwik\Db\Adapter\Pdo\Mysql::resetConfig() (which lead to: Circular dependency detected while trying to resolve entry 'Piwik\Plugins\CorePluginsAdmin\Controller')

Cheers
Kiwi-X

@kiwi-x commented on March 26th 2018

Hi,

I just updated the core/Db/Adapter/MysqlTrait.php and now it works as expected again 👍

From my point of view the problem seems to be solved.

Thank you!

Cheers
Kiwi-X

@robocoder commented on April 4th 2018 Contributor

Here's the simpler PR per @diosmosis review.

@robocoder commented on April 4th 2018 Contributor

And just for the record, back when I added the Mysqli adapter, we were still supporting older versions of PHP which didn't support traits, hence the duplicated code.

@diosmosis commented on April 4th 2018 Member

I'm not sure, but from your comment it sounds like I may have come across as argumentative. I'm not trying to point fingers or blame anyone about anything, apologies if that's what it sounds like. Personally, I have no real issue w/ the duplicated code here, since the code is rarely modified. I mostly want to avoid design decisions based entirely on DRY, as I've caused problems doing that myself in the past.

Will take a look & test this tomorrow.

@diosmosis commented on April 5th 2018 Member

Tested:

  • that it works when deployed on azure
  • that the version_compare call works properly w/ the funky mariadb version string

LGTM, will add a comment & merge w/ 3.x for the build.

@diosmosis commented on April 5th 2018 Member

Would like to merge, but want to wait for a second opinion from @matomo-org/core-team in case I'm missing something.

Also, this may fix https://github.com/matomo-org/matomo/issues/11934 too

@robocoder commented on April 19th 2018 Contributor

LGTM. Yes, imho this would fix #11934 too. Any reason this can't make it into 3.5.0?

@diosmosis commented on April 19th 2018 Member

It can, I haven't had time to merge anything other than those required by customers. Will merge it now.

@diosmosis commented on April 19th 2018 Member

Ok, there are some UI tests that need updated screenshots (like https://builds-artifacts.matomo.org/matomo-org/matomo/3.x-dev/26861/Dashboard_removed.png), will have to fix those first (after merging w/ 3.x-dev). Which will have to wait until I or someone else has time to merge.

@mattab commented on April 23rd 2018 Member

Nicely done @robocoder @kiwi-x @diosmosis :tada:

This Pull Request was closed on April 23rd 2018
Powered by GitHub Issue Mirror