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
Conversation
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. |
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 Sorry. I will try to debug the both methods. Thanks for your work so far. Regards |
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" |
Maybe the data is cached somewhere. You can try to clear the tmp directory of Matomo to clear the file caches... Or run a |
Hi, 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] 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 |
Sorry. To make this work, you need to comment out the Line 55 in 67f78ce
but that re-opens an information disclosure risk. |
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 |
I just thought of a workaround. I'll implement later this evening. |
I added an |
c482d07
to
0ed92fd
Compare
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] 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 |
fbc741a
to
39525fa
Compare
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 |
core/Db/Adapter/MysqlTrait.php
Outdated
} | ||
} | ||
|
||
return parent::getServerVersion(); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
core/Db/Adapter/Mysqli.php
Outdated
*/ | ||
class Mysqli extends Zend_Db_Adapter_Mysqli implements AdapterInterface | ||
{ | ||
use MysqlTrait; |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.)
39525fa
to
22c0cd9
Compare
Here's the simpler PR per @diosmosis review. |
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. |
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. |
Tested:
LGTM, will add a comment & merge w/ 3.x for the build. |
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 |
LGTM. Yes, imho this would fix #11934 too. Any reason this can't make it into 3.5.0? |
It can, I haven't had time to merge anything other than those required by customers. Will merge it now. |
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. |
Nicely done @robocoder @kiwi-x @diosmosis 🎉 |
…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.
No description provided.