Description

This projects allows to set the cookie name/value condition & use it for block visibility or any other logic that is based on top of condition plugins D8 core system.

E.g. we use it for layout auto selection with patch provided by me here: https://www.drupal.org/project/layout_library/issues/3089493

Project link

https://www.drupal.org/project/cookie_condition

Git instructions

git clone --branch 8.x-1.x https://git.drupalcode.org/project/cookie_condition

Pareview Checklist

https://pareview.sh/pareview/https-git.drupal.org-project-cookie_conditi...

Comments

Pavel Ruban created an issue. See original summary.

maheshkp92’s picture

maheshkp92
Primary language English
Location Mumbai
commented
Issue summary: View changes

Thank you for applying! I added the Git instructions for non-maintainer users and the PAReview checklist link. Reviewers will check the project and post comments to list what should be changed.

If you haven't done it, yet, please check the PAReview report and fix what needs to be fixed. There could be some false positives; verify that what reported is correct, before making any change.

maheshkp92’s picture

maheshkp92
Primary language English
Location Mumbai
commented

Please remove the invalid branch called "@branch".

maheshkp92’s picture

maheshkp92
Primary language English
Location Mumbai
commented
Status: Needs review » Needs work
avpaderno’s picture

avpaderno
he/him
Primary language Italian
Location Brescia, 🇮🇹 🇪🇺
commented
Issue tags: -condition plugins +PAreview: single application approval
pavel ruban’s picture

pavel ruban commented
Issue summary: View changes
pavel ruban’s picture

pavel ruban commented

Thank you for review,

- I've fixed PARreview points, sorry forgot to run sniffer.
- Removed @branch, wasn't able before as it was default gitlab branch, found setting on project edit page.
- I added new release with all those fixes & adjusted default branch to 8.x-1.x one.

If it still requires any further amends please me know,

Thanks in advance.

pavel ruban’s picture

pavel ruban commented
Status: Needs work » Needs review
batkor’s picture

batkor
Location Irkutsk
commented

i think better get session from request instead global variable $_COOKIE
Example:

   $cookies = \Drupal::request()->cookies;
    if ($cookies->get($cookie_name) === $this->configuration['cookie_value']) {
      return TRUE;
    }

Up:
Use DependencyInjection ?

chi’s picture

chi commented

Nitpick:

if (isset($_COOKIE[$cookie_name]) && $_COOKIE[$cookie_name] === $this->configuration['cookie_value']) {
  return TRUE;
}

return FALSE;

This could be written in a single line.

return \Drupal::request()->cookies->get($cookie_name) === $this->configuration['cookie_value'];
pavel ruban’s picture

pavel ruban commented

Fixed, I ended with this solution:

https://git.drupalcode.org/project/cookie_condition/commit/cc2b867

  public function evaluate() {
    if (empty($this->configuration['cookie_name']) && !$this->isNegated()) {
      return TRUE;
    }

    $cookie_name = $this->configuration['cookie_name'];
    $cookie_value = $this->configuration['cookie_value'];

    $cookies = $this->requestStack->getCurrentRequest()->cookies;

    return $cookies->get($cookie_name) === $cookie_value;
  }
chi’s picture

chi commented

if (empty($this->configuration['cookie_name']) && !$this->isNegated()) {
Since you defined $cookie_name it makes sense to use in this line as well. Also consider using dependency injections for request as suggested in #9.

pavel ruban’s picture

pavel ruban commented

pushed

    $cookie_name = $this->configuration['cookie_name'];
    $cookie_value = $this->configuration['cookie_value'];

    if (!$cookie_name && !$this->isNegated()) {
      return TRUE;
    }

>> Also consider using dependency injections
Sorry I already implemented that & updated the comment above with the latest code, we just did it simultaneously

chi’s picture

chi commented

Why is the 'core' key commented out?
https://git.drupalcode.org/project/cookie_condition/blob/8.x-1.x/cookie_...

chi’s picture

chi commented

By the way. Have you does this plugin work well with cache?

If no, you probably can fix it by implementing ::getCacheContexts() method like this. 


/**
 * {@inheritdoc}
 */
public function getCacheContexts() {
  $contexts = parent::getCacheContexts();
  $contexts[] = 'cookies:' . $this->configuration['cookie_name'];
  return $contexts;
}

Note I haven't checked this code myself.

chi’s picture

chi commented

Nitpick:

"Allows to select cookie & it's value."

I think it should be written in more formal way. Meaning 'and' is preferable over '&'. Though I am not aware if there is a coding standard on this.
Also "it's" needs to be "its".

pavel ruban’s picture

pavel ruban commented

Thanks Chi, cache remark is really good, thanks for input.

pushed

  /**
   * {@inheritdoc}
   */
  public function getCacheContexts() {
    $contexts = parent::getCacheContexts();
    if ($this->configuration['cookie_name']) {
      $contexts[] = 'cookies:' . $this->configuration['cookie_name'];
    }
    return $contexts;
  }

#core usually is added to info files & then gets cached by packaging module script on DO & comments the initial defined field out, I went through all modules I have & all of them have this commented key.

I think that I haven't to add core tag at all (apart of implementing a custom module case) & it will be automatically added during deploy, so I deleted it completely & tested with relase, after that it works right I guess

pavel ruban’s picture

pavel ruban commented

"Allows to select cookie & it's value."

Fixed:

 * Pass checks if cookie is present and its value equals to a defined one.
 *
 * @Condition(
 *   id = "cookie",
 *   label = @Translation("Cookie"),
 *   description = @Translation("Allows to select a particular cookie with a specific value.")
chi’s picture

chi commented 
return t('Cookie "@name" is @negate"@value"', [
  '@name' => $this->configuration['cookie_name'],
  '@value' => $this->configuration['cookie_value'],
  '@negate' => $this->isNegated() ? 'NOT ' : '',
]);

It will be hard for translators to understand this phrase because it has more placeholders than translatable words. Consider using a separate message when the condition is negated. Also $this->t can be used instead of t.

chi’s picture

chi commented

I think that I haven't to add core tag at all (apart of implementing a custom module case) & it will be automatically added during deploy, so I deleted it completely & tested with relase, after that it works right I guess

Now it is not possible to install the module from git.

Also consider specifying core_version_requirement.
https://www.drupal.org/node/3070687

pavel ruban’s picture

pavel ruban commented

pushed: 

   $placeholders = [
      '@name' => $this->configuration['cookie_name'],
      '@value' => $this->configuration['cookie_value'],
    ];

    return $this->isNegated()
      ? $this->t('Cookie "@name" is NOT equal to "@value"', $placeholders)
      : $this->t('Cookie "@name" has "@value" value', $placeholders);

name: Cookie Condition
type: module
description: Provides a Condition plugin that checks to see if a particular cookie has a specific value.
core: 8.x
core_version_requirement: ^8
package: Other
chi’s picture

chi commented
Status: Needs review » Reviewed & tested by the community

Looks good to me now.

Though it would be nice to have an automated test for the plugin.

avpaderno’s picture

avpaderno
he/him
Primary language Italian
Location Brescia, 🇮🇹 🇪🇺
commented
Assigned: Unassigned » avpaderno
Status: Reviewed & tested by the community » Fixed

Thank you for your contribution! I am going to update the project to opt it into security coverage.

These are some recommended readings to help with excellent maintainership:

You can find more contributors chatting on the IRC #drupal-contribute channel. So, come hang out and stay involved.
Thank you, also, for your patience with the review process.
Anyone is welcome to participate in the review process. Please consider reviewing other projects that are pending review. I encourage you to learn more about that process and join the group of reviewers.

I thank all the dedicated reviewers as well.

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.