-
-
Notifications
You must be signed in to change notification settings - Fork 23
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
WIP: Implement tracking of page title #251
base: develop
Are you sure you want to change the base?
WIP: Implement tracking of page title #251
Conversation
This column will be used for aggregation and is filled with 1 initially. The number of hits for a specific date/target/referrer is thus no longer the number of entries, but the sum of all hits.
We now aggregate statistics data in the cleanup routine preserving only distinct entries for each date/referrer/target combination with the sum of all hits.
If Statify is extended by custom columns the aggregation routine will fail. To support such scenarios, we introduce a boolean hook, so the previous behavior can be preserved without breaking compatibility.
inc/class-statify-frontend.php
Outdated
|
||
$meta_value = call_user_func( $sanitize_function, $meta_value ); | ||
|
||
/* Init rows */ |
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.
Single line comments should not use block comments.
inc/class-statify-schema.php
Outdated
} | ||
} | ||
|
||
/* neue option: db Version 2.0.0 */ |
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.
Remove this comment.
48a5137
to
f800357
Compare
* | ||
* @var string[] | ||
*/ | ||
public static $tables = array( |
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.
Maybe this should not be public and/or constant?
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.
Const array is PHP 5.6+ iirc (currently supported 5.3+)
But feel free to raise the requirements, if that make things easier here.
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.
Constant arrays is not a thing at all 😆 Like not before PHP 7+, so we just make it private.
Just a reminder that this change conflicts with the planned aggregation (#234) or at least requires significant changes on at least one side. |
f800357
to
8df7cd1
Compare
@stklcode it is true that this might conflict with the data aggregation. But not necessarily. We haved discussed the data aggregation being in conflict with multiple other issues and have to yet decided if, and how the data aggregation would be done. We could still have aggregated data while still collecting more information. We "only" need to decide "at aggregation" how to handle this aggregated data. Would we aggregate per unique touple, use just one (the latest) additional data set? This should then be discussed in the data aggregation ticket. |
I didn’t say that aggregation is impossible with additional metadata, just not in the currently proposed way, i.e. we cannot simply merge both without additional work. The title typically has 1-to-1 relation with the target URL at at specific point in time, so this case is more or less trivial to solve. With more attributes combinations grow and the benefits of tuple aggregation is thus reduced, so multi-layer aggregation over each attribute may be worth thinking of. But this would be incompatible with the proposed data structure. But all this might be overthought at this time, we should not make it more complex than necessary for the first shot. |
8df7cd1
to
210a515
Compare
Kudos, SonarCloud Quality Gate passed! 0 Bugs No Coverage information |
0d94eac
to
e3d991a
Compare
0d94eac
to
e3d991a
Compare
Kudos, SonarCloud Quality Gate passed! 0 Bugs No Coverage information |
Is this still in the pipeline? |
User has a feature request to show the page title instead of just the target url. After some long discussions we decided to introduce a new statify meta table. In this table any additional tracking data can be saved. With this PR we save the wp_document_title in this meta table.
Fixes #78 , #196