Support from Acquia helps fund testing for Drupal Acquia logo

Comments

mikeegoulding created an issue. See original summary.

mikeegoulding’s picture

nkoporec’s picture

Version: 8.x-1.0-beta1 » 8.x-1.x-dev
Status: Active » Needs work

Hey tested your module, everything seems to be working.It only has one coding standard issue, in settings form you should use $this->t(), instead of t().

mikeegoulding’s picture

Good catch. I think I have that updated here.

mikeegoulding’s picture

Status: Needs work » Needs review
nkoporec’s picture

Status: Needs review » Needs work

Hey, the patch doesn't apply on the 8.x-1.x branch(using latest drupal 8.5) ... when checking the patch it throws this error:

error: patch failed: src/Form/SettingsForm.php:55
error: src/Form/SettingsForm.php: patch does not apply
mikeegoulding’s picture

Ah, my bad! I kinda did a rush find replace on that one. This patch applies cleanly.

nkoporec’s picture

Status: Needs work » Reviewed & tested by the community

Hi @mikeegoulding, tested the latest patch and applies cleanly.Good job!

kristiaanvandeneynde’s picture

Status: Reviewed & tested by the community » Needs work
  1. +++ b/config/schema/session_limit.schema.yml
    @@ -11,4 +11,6 @@ session_limit.settings:
    \ No newline at end of file
    

    Several files have been stripped from their ending newline. Please check your IDE and make sure that it always adds a newline at the end of a file.

  2. +++ b/session_limit.install
    @@ -2,5 +2,15 @@
    +   */
    +
    +  function session_limit_update_8001() {
    +
    +    $config_factory = \Drupal::configFactory();
    

    Remove this empty line at the start of the function and between the function declaration and docblock.

  3. +++ b/session_limit.install
    @@ -2,5 +2,15 @@
    +    $config->set('session_limit_logged_out_display_message', t('You have been automatically logged out. Someone else has logged in with your username and password and the maximum number of @number simultaneous session(s) was exceeded. This may indicate that your account has been compromised or that account sharing is not allowed on this site. Please contact the site administrator if you suspect your account has been compromised.'));
    

    Not sure you should use t() here. The settings Yaml file requires the string to be specified in the default language (which is often EN)

  4. +++ b/src/Form/SettingsForm.php
    @@ -49,38 +49,45 @@ class SettingsForm extends ConfigFormBase {
    +      '#default_value' => \Drupal::config('session_limit.settings')->get('session_limit_logged_out_display_message'),
    

    Please properly inject this service.

  5. +++ b/src/Services/SessionLimit.php
    @@ -470,8 +470,8 @@ class SessionLimit implements EventSubscriberInterface {
    +    // @todo get rid of these statics.
    +    return \Drupal::config('session_limit.settings')
    

    You even have a @todo for it, so please get rid of these and properly inject them.

mikeegoulding’s picture

Alright. I think I've made these changes. I was originally trying to not make any major changes to other sections. Should have done the dependency injection from the start.

mikeegoulding’s picture

Status: Needs work » Needs review
mikeegoulding’s picture

Status: Needs review » Needs work

And I just realized my patch is blank and there are new commits on this that add some of the dependency injection mentioned in previous comments. I'll have to fix this up later.

mikeegoulding’s picture

Re rolling patch against latest 8.1-1.x branch

avinash_odal’s picture

The Session Limits does not work any more, tested on Drupal 8-3-5 and Drupal 8-5-3. It was working with Version 8-2-x versions of drupal, however i tested with the 8-3-x branch and the 8-5-x branch and the session limit doesn't work.

mike.vindicate’s picture

Made the patch apply to the latest version.

idebr’s picture

+++ b/config/schema/session_limit.schema.yml
@@ -14,3 +14,5 @@ session_limit.settings:
+    session_limit_logged_out_display_message:
+      type: string

type: string is not translatable. Let's use type: label so the value can be translated through Configuration translation. Documentation about these data types is available at https://www.drupal.org/docs/8/api/configuration-api/configuration-schema...

mike.vindicate’s picture

Changed type to label.
@idebr thanks for pointing it out.

mike.vindicate’s picture

Attached patch which can be applied to the latest version 2.0.0-beta2.

mike.vindicate’s picture

Something went wrong with the creation of the patch in #18, missing the settings form.
Attached new patch with the added form element and saving of the value.

Ambient.Impact’s picture

Would it be possible to make the text content filtered text? For example, if I want to add a link to a contact form in the message, this is not possible with the patch in #19 as it just outputs it as plain text.

Sophie.SK’s picture

This should probably be rolled against the latest version (2.x-dev) rather than the 8.x-dev branch, though this patch does apply to the latest 2.0 beta too.

It would probably be good to have the default text in the form too. If an admin doesn't make a change but saves the form, then it shouldn't override in config. I'll make some changes to the patch.

Noticed that it's there in the install config, but I already had the module installed, and hadn't run updates. Doh!

Sophie.SK’s picture

Version: 8.x-1.x-dev » 2.x-dev

Hm, having spent a while trying to figure it out, I can't get the formatting to work right on text output. Sorry - going to mark this as needs work but against the current version.

yasmeensalah’s picture

Re-roll the patch to make it work with this patch https://www.drupal.org/i/2985067