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

On Azure fallback to "select @@version" #12551

Merged
merged 3 commits into from Apr 23, 2018

Conversation

robocoder
Copy link
Contributor

No description provided.

@sgiehl
Copy link
Member

sgiehl commented Feb 12, 2018

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

@mattab mattab modified the milestones: 3.3.1, 3.4.0, Priority Backlog (Help wanted) Mar 19, 2018
@mattab
Copy link
Member

mattab commented Mar 22, 2018

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
Copy link

kiwi-x commented Mar 22, 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
Copy link

kiwi-x commented Mar 22, 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
Copy link
Member

sgiehl commented Mar 22, 2018

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
Copy link

kiwi-x commented Mar 22, 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
Copy link
Contributor Author

Sorry. To make this work, you need to comment out the $adapter->resetConfig(); from

$adapter->resetConfig();

but that re-opens an information disclosure risk.

@kiwi-x
Copy link

kiwi-x commented Mar 23, 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
Copy link
Contributor Author

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

@robocoder
Copy link
Contributor Author

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

@robocoder robocoder force-pushed the patch-12002-12030 branch 2 times, most recently from c482d07 to 0ed92fd Compare March 24, 2018 22:25
@kiwi-x
Copy link

kiwi-x commented Mar 26, 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
Copy link

kiwi-x commented Mar 26, 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

@mattab mattab modified the milestones: Priority Backlog (Help wanted), 3.4.1 Mar 27, 2018
}
}

return parent::getServerVersion();
Copy link
Member

Choose a reason for hiding this comment

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

Would 'SELECT @@VERSION' work in every case? Wondering if we could just use that to avoid having to check for azure in the first place.

From what I can tell, getServerVersion() isn't used during tracking or aggregation, so I wouldn't expect there to be a performance impact.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If it's MySQL compatible, SELECT @@VERSION should always work. It might look funny with MariaDB though.

*/
class Mysqli extends Zend_Db_Adapter_Mysqli implements AdapterInterface
{
use MysqlTrait;
Copy link
Member

Choose a reason for hiding this comment

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

I don't like the use of a trait in this PR, where it seems to be just a place to gather an assortment of methods two classes have in common. It violates the single responsibility principle and is thus hard to understand exactly what it represents (evidenced by the generic name, "MysqlTrait").

@tsteur @sgiehl do you have the same feeling I do? or are you guys ok w/ this use of a trait?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Perhaps MySqlAdapterTrait would be a better name but this trait is how traits were meant to be used. It also fulfills the DRY principle.

Traits are a mechanism for code reuse in single inheritance languages such as PHP. A Trait is intended to reduce some limitations of single inheritance by enabling a developer to reuse sets of methods freely in several independent classes living in different class hierarchies.

Piwik\Db\Adapter\Mysqli extends Zend_Db_Adapter_Mysqli while Piwik\Db\Adapter\Pdo\Mysql extends Zend_Db_Adapter_Pdo_Mysql.

Copy link
Member

Choose a reason for hiding this comment

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

I understand the technical purpose of traits and of inheritance, but using them just to separate out common code is (for lack of a better term) lower order design. It results in code that doesn't have a well defined purpose, and thus loses reusability and coherence (there's a reason "composition over inheritance" is a thing, it forces you to think further than DRY). I've made this mistake before, one good example being: https://github.com/matomo-org/matomo/blob/3.x-dev/core/BaseFactory.php. This class had limited use at the time, no use now, and is far less powerful than DI.

The point I'm trying to make isn't that we shouldn't use traits, but that small refactors like this done simply to reduce the amount of code have a tendency to become cruft in the future. I think ideally, if we want to reduce the redundancy in these two classes, we'd think about it in a separate issue/PR rather than attach it to a bug fix for expedience.

In this specific PR, the hasBlobDataType() and related methods don't really need to be moved, they're simple enough that removing the redundancy won't make the code easier to understand (in fact I would say it would do the opposite, since now you can't see where those methods are implemented at a glance). What's important from a code coherence standpoint is separating & reusing the logic to detect & handle azure, into some unit that exists specifically to encapsulate azure-mysql logic. I hope that makes sense. (And I don't necessarily speak for other devs.)

@robocoder
Copy link
Contributor Author

Here's the simpler PR per @diosmosis review.

@robocoder
Copy link
Contributor Author

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
Copy link
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
Copy link
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
Copy link
Member

diosmosis commented Apr 5, 2018

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 #11934 too

@robocoder
Copy link
Contributor Author

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

@diosmosis
Copy link
Member

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

@diosmosis
Copy link
Member

diosmosis commented Apr 19, 2018

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 mattab merged commit 718eb0e into matomo-org:3.x-dev Apr 23, 2018
@mattab
Copy link
Member

mattab commented Apr 23, 2018

Nicely done @robocoder @kiwi-x @diosmosis 🎉

@mattab mattab changed the title Fixes #12002 and #12030 - azure fallback to "select @@version" On Azure fallback to "select @@version" May 8, 2018
InfinityVoid pushed a commit to InfinityVoid/matomo that referenced this pull request Oct 11, 2018
…ct @@Version" (matomo-org#12551)

* Fixes matomo-org#12002 and matomo-org#12030 - azure fallback to "select @@Version"

* Add comment for why @@Version is used instead of going through the connection object.
@robocoder robocoder deleted the patch-12002-12030 branch March 26, 2022 16:41
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.

None yet

5 participants