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

Updated CookieConsent to version 3.0.0. and some minor updates #4

Open
wants to merge 24 commits into
base: master
Choose a base branch
from

Conversation

rahkapetteri
Copy link

@rahkapetteri rahkapetteri commented Mar 8, 2024

Hi!

I think Cookie Consent 3.0.0 is working properly now 👨‍💻

Additional changes:

  • Added the option to declare cookies (even with regex variables) so cookies are getting deleted after changing consent
  • Fixed issues with embedded content when changing consent. Before the fix, when consent was removed, I could still see the embeds
  • Updated documentation
  • Updated Iframe Manager to 1.2.5
  • Added cc_link CSS-style to assets/cookieconsent.css
  • Fixes Why send consent POST on each request? #2

If you have any questions about the fixes I’m happy to discuss them 😊

Copy link

height bot commented Mar 8, 2024

Link Height tasks by mentioning a task ID in the pull request title or commit messages, or description and comments with the keyword link (e.g. "Link T-123").

💡Tip: You can also use "Close T-X" to automatically close a task when the pull request is merged.

Comment on lines 61 to +67
/**
* Github updater.
* Github updater. Disabled by commenting out.
*
* @since 0.1.0
*/
require plugin_base_path() . '/plugin-update-checker/plugin-update-checker.php';
$update_checker = \Puc_v4_Factory::buildUpdateChecker( 'https://github.com/digitoimistodude/air-cookie', __FILE__, 'air-cookie' );
// require plugin_base_path() . '/plugin-update-checker/plugin-update-checker.php';
// $update_checker = \Puc_v4_Factory::buildUpdateChecker( 'https://github.com/digitoimistodude/air-cookie', __FILE__, 'air-cookie' );
Copy link
Member

Choose a reason for hiding this comment

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

These kinds of changes shouldn't be included in PRs 😃

Copy link
Author

Choose a reason for hiding this comment

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

Was meant for local only 🤦‍♂️

Copy link
Member

Choose a reason for hiding this comment

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

Haha, happens to the best of us.

Comment on lines -29 to 58
'cookie_name' => 'air_cookie',
'revision' => $categories_version, // use version number to invalidate if categories change
// 'theme_css' => plugin_base_url() . '/assets/cookieconsent.css',
'cookie_expiration' => 182, // in days, 182 days = 6 months
'auto_language' => false,
'current_lang' => $lang,
'autorun' => true,
'page_scripts' => true,
'delay' => '0',
'gui_options' => [
'consent_modal' => [
'layout' => 'box',
'position' => 'bottom left',
'revision' => $categories_version, // use version number to invalidate if categories change
'cookie' => [
'name' => 'air_cookie',
'expiresAfterDays' => 182, // in days, 182 days = 6 months
],

'guiOptions' => [
'consentModal' => [
'layout' => 'box wide',
'position' => 'bottom left',
'equalWeightButtons' => false,
'flipButtons' => false,
],

'preferencesModal' => [
'layout' => 'box',
'equalWeightButtons' => true,
'flipButtons' => false,
],
],

'language' => [
'default' => $lang,
'translations' => [
$lang => []
]
],

'categories' => [],
];
Copy link
Member

Choose a reason for hiding this comment

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

Refactoring the settings object isn't a good practice as this will break backwards compability pretty much just for style points.

We name variables according to the WordPress Coding Standard's Naming Conventions meaning snake_case instead of camelCase.

Copy link
Member

Choose a reason for hiding this comment

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

Changes are from CookieConsent, ahh.

Copy link
Member

Choose a reason for hiding this comment

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

I'd still prefer at least mapping the old keys to the new ones if they are set 🤔

Copy link
Author

Choose a reason for hiding this comment

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

I´ll look into it.

@@ -60,13 +61,68 @@ function my_add_cookie_category( $categories ) {
When adding new categories, the function itself is responsile for handling the translations for title and description.

There is also `air_cookie\categories\{category-key}` filter available to change the settings of indivual category.
```php
add_filter( 'air_cookie\categories\{analytics}', 'my_change_category_analytics' );
Copy link
Member

Choose a reason for hiding this comment

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

It would be air_cookie\categories\analytics, without the braces.

Copy link
Author

@rahkapetteri rahkapetteri May 12, 2024

Choose a reason for hiding this comment

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

Hmm I can´t get it to work without brackets. Only way to use the filter without brackets would require to change in settings.php:

$categories[ $key ] = apply_filters( "air_cookie\categories\{$category_key}", $category );

to $categories[ $key ] = apply_filters( "air_cookie\categories\\{$category_key}", $category );

Atleast that´s how PHP-docs say to handle backslashed variables 🤔

// Won't work, outputs: C:\folder\{fantastic}.txt
echo "C:\folder\{$great}.txt"

// Works, outputs: C:\folder\fantastic.txt
echo "C:\\folder\\{$great}.txt"

Copy link
Member

Choose a reason for hiding this comment

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

Not in the code, I meant in the example 😄

If user wants to filter analytics they need to create a filter add_filter ( 'air_cookie\categories\analytics', 'my_change_analytics' );, which is then called by apply_filters( "air_cookie\categories\{$category_key}", $category );.

"air_cookie\categories\{$category_key}" gets resolved to "air_cookie\categories\analytics as {$category_key} is a string template.

Copy link
Member

@raikasdev raikasdev Jul 10, 2024

Choose a reason for hiding this comment

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

This is indeed a bug. #10

Comment on lines 35 to +45
if ( ! is_array( $settings ) ) {
return;
return;
}

// Cookie Consent javascript base.
wp_enqueue_script( 'cookieconsent', plugin_base_url() . '/assets/cookieconsent.js', [], get_script_version(), false );
wp_enqueue_script( 'cookieconsent', plugin_base_url() . '/assets/cookieconsent.js', [], get_script_version(),
array(
'in_footer' => true,
'strategy' => 'defer',
)
);
Copy link
Member

Choose a reason for hiding this comment

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

What is happening here with the whitespace?

Copy link
Author

Choose a reason for hiding this comment

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

Probably VS Code auto indentation with different settings 😅

Comment on lines +41 to +45
array(
'in_footer' => true,
'strategy' => 'defer',
)
);
Copy link
Member

Choose a reason for hiding this comment

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

We use the short [] syntax instead of array().

Comment on lines +108 to +127
onFirstConsent: () => {
ccOnAccept();
},

onConsent: () => {
checkIframeConsent();
},

onModalShow: () => {
<?php if ( apply_filters( 'air_cookie\styles\set_max_width', true ) ) : ?>
var cookieconsent_element = document.querySelector('div#cc-main div.cm');
if( typeof( cookieconsent_element ) != 'undefined' && cookieconsent_element != null ) {
cookieconsent_element.style = 'max-width: 40em;';
}
<?php endif; ?>
},

onChange: () => {
ccOnChange();
}
Copy link
Member

Choose a reason for hiding this comment

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

onFirstConsent, onConsent and onChange probably don't need to be inside a arrow function and can be passed as arguments.

onChange: ccOnChange.

@@ -145,8 +204,7 @@ function do_category_js( $category ) {
$event_key = "air_cookie_{$category_key}";

ob_start(); ?>

if ( cc.allowedCategory( '<?php echo $category_key; // phpcs:ignore WordPress.Security.EscapeOutput.OutputNotEscaped ?>' ) ) {
if ( CookieConsent.getCookie( 'categories' ).includes( '<?php echo $category_key; // phpcs:ignore WordPress.Security.EscapeOutput.OutputNotEscaped ?>' ) ) {
Copy link
Member

Choose a reason for hiding this comment

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

Why do we have to use this instead of CookieConsent.acceptedCategory()?

Copy link
Author

Choose a reason for hiding this comment

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

Wasn't working correctly in 3.0.0, but I think that´s fixed in 3.0.1 now (PR is 3.0.0).

Copy link
Member

Choose a reason for hiding this comment

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

Ah!

Comment on lines +53 to +59
// Check if the index exists
$index_exists = $wpdb->get_row( "SHOW INDEX FROM {$table_name} WHERE Key_name = 'idx_id_revision_value'" ); // phpcs:ignore

// CREATE INDEX idx_id_revision_value ON wp_air_cookie (visitor_id, cookie_revision, cookie_value);
if ( null === $index_exists ) {
$wpdb->query( "CREATE INDEX idx_id_revision_value ON {$table_name} (visitor_id, cookie_revision, cookie_value);" ); // phpcs:ignore
}
Copy link
Member

Choose a reason for hiding this comment

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

Does this have to happen on every request or could this just be ran when the plugin is activated or if the version has changed?

@rahkapetteri
Copy link
Author

Thanks for comments! I´ll look into those.

I was doing some testing with fork repo and didn´t notice latest changes went to PR also.

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.

Why send consent POST on each request?
2 participants