Hi,

Is there plans to introduce Drupal 8 version of this module?. I have been working in a version for Drupal 8, I going to leave my patch in the comment.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

gvso’s picture

Status: Active » Needs work
FileSize
46.93 KB

Here is my patch. Everything seems working well but the autocomplete for terms_of_use_node_title doesn't work.

terms_of_use.autocomplete is the routing configuration for the path /terms_of_use/autocomplete and called Drupal\terms_of_use\AutocompleteController localizated in src/AutocompleteController.php.

When a request for the path /terms_of_use/autocomplete is done, it returns a Drupal 404 error. Try http://base_url/terms_of_use/autocomplete.

gvso’s picture

I forgot about this. This patch works correctly, but only enter a nid is valid..

gvso’s picture

Title: Port to Drupal 8 » Drupal 8 version
Issue summary: View changes
gvso’s picture

Title: Drupal 8 version » Terms of Use version for Drupal 8
larowlan’s picture

  1. diff --git a/CHANGELOG.txt b/CHANGELOG.txt
    deleted file mode 100644
    
    deleted file mode 100644
    index cca46f8..0000000
    
    index cca46f8..0000000
    --- a/CHANGELOG.txt
    
    --- a/CHANGELOG.txt
    +++ /dev/null
    
    +++ /dev/null
    @@ -1,23 +0,0 @@
    
    @@ -1,23 +0,0 @@
    -
    -January 17, 2011
    ---------------------
    

    Please add this file back - and add an entry for your work :)

  2. +++ /dev/null
    --- /dev/null
    +++ b/LICENSE.txt
    
    +++ b/LICENSE.txt
    +++ b/LICENSE.txt
    @@ -0,0 +1,339 @@
    
    @@ -0,0 +1,339 @@
    +                    GNU GENERAL PUBLIC LICENSE
    +                       Version 2, June 1991
    

    This should not be in the repo, d.o packager auto-adds it, please remove

  3. +++ b/LICENSE.txt
    --- /dev/null
    +++ b/config/install/terms_of_use.settings.yml
    
    +++ b/config/install/terms_of_use.settings.yml
    +++ b/config/install/terms_of_use.settings.yml
    @@ -0,0 +1,2 @@
    
    @@ -0,0 +1,2 @@
    +terms_of_use_fieldset_name: 'Terms of Use'
    

    You also need a terms_of_use.schema.yml too

    I wouldn't prefix the variables with terms_of_use anymore, as they are already namespaced by the filename

  4. +++ b/config/install/terms_of_use.settings.yml
    --- /dev/null
    +++ b/css/terms_of_use.css
    
    +++ b/css/terms_of_use.css
    +++ b/css/terms_of_use.css
    @@ -0,0 +1,4 @@
    
    @@ -0,0 +1,4 @@
    +#content #terms-of-use.content {
    +  height: 250px;
    +  overflow: auto;
    +}
    \ No newline at end of file
    

    Needs a newline on the last line

  5. +++ b/src/Form/TermsOfUseForm.php
    @@ -0,0 +1,186 @@
    +      '#default_value' => $this->t( $config->get('terms_of_use_node_title') ),
    ...
    +      '#default_value' => $this->t($config->get('terms_of_use_fieldset_name')),
    

    Don't pass config values through $this->t(), add a schema and they can be translated via config_translation module

  6. +++ b/src/Form/TermsOfUseForm.php
    @@ -0,0 +1,186 @@
    +      '#description' => $this->t( 'Node <em>title</em> of the page or story (or blog entry or book page) where your Terms of Use are published.' ),
    

    Extra space here after (

  7. +++ b/src/Form/TermsOfUseForm.php
    @@ -0,0 +1,186 @@
    +        $node = node_load($nid);
    

    use Node::load instead

  8. +++ b/src/Form/TermsOfUseForm.php
    @@ -0,0 +1,186 @@
    +          $form['terms_of_use_text']['terms_of_use_node_title'] = $node->title;
    

    $node->label() now, $node->title is a FieldItemListInterface object, not a scalar

  9. +++ b/src/Form/TermsOfUseForm.php
    @@ -0,0 +1,186 @@
    +      $nid = db_select('node', 'n')
    

    You can inject the db connection and use $this->database->select() instead of the procedural wrapper

  10. +++ b/src/Form/TermsOfUseForm.php
    @@ -0,0 +1,186 @@
    +        form_set_error('terms_of_use_node_title', t('No post was published with this title.'));
    

    form_set_error is gone too, use $form_state->setErrorByName()

  11. +++ b/src/Form/TermsOfUseForm.php
    @@ -0,0 +1,186 @@
    +        variable_set('terms_of_use_node_id', $nid);
    

    variable_set is gone - this should be replaced with config now

  12. +++ b/src/Form/TermsOfUseForm.php
    @@ -0,0 +1,186 @@
    +    $config->set('terms_of_use_node_id', $node_id)
    +          ->set('terms_of_use_node_title', $node_title)
    

    These two aren't present in the default settings.yml file, please add them

  13. +++ b/src/Form/TermsOfUseForm.php
    @@ -0,0 +1,186 @@
    +    $form_build_id = $_POST['form_build_id'];
    

    please don't use $_POST direct, use $form_state->getValue()

  14. +++ b/terms_of_use.module
    @@ -1,73 +1,47 @@
    +function terms_of_use_form_user_register_form_alter(&$form, \Drupal\Core\Form\FormStateInterface $form_state, $form_id) {
    

    add a use statement and use the alias in the argument signature instead

  15. +++ b/terms_of_use.module
    @@ -1,73 +1,47 @@
    +  $langcode = \Drupal::languageManager()->getLanguage(\Drupal\Core\Language\Language::TYPE_CONTENT);
    

    add a use statement and use the Language alias instead

  16. +++ b/terms_of_use.module
    @@ -1,73 +1,47 @@
    +    $fieldset_name = \Drupal\Component\Utility\Xss::filter($config->get('terms_of_use_fieldset_name'));
    

    Add a use statement and use the XSS short-hand alias instead

  17. +++ b/terms_of_use.module
    @@ -1,73 +1,47 @@
    +    // Getting the node through its nid
    

    nitpick: needs a trailing .

  18. +++ b/terms_of_use.module
    @@ -1,73 +1,47 @@
         $node = node_load($terms_of_use_node_id);
    

    Node::load now

  19. +++ b/terms_of_use.module
    @@ -77,27 +51,27 @@ function terms_of_use_form_user_register_form_alter(&$form, $form_state) {
    +      elseif ( $items = $translation->get('body') ) {
    ...
    +        $form['terms_of_use']['terms_of_use_text'] = $node->body[0]->view('full');
    

    $translation->body->processed should be used instead

  20. +++ b/terms_of_use.module
    @@ -77,27 +51,27 @@ function terms_of_use_form_user_register_form_alter(&$form, $form_state) {
    +        //_drupal_add_library('terms_of_use/terms_of_use');
    

    remove this

  21. +++ b/terms_of_use.routing.yml
    @@ -0,0 +1,14 @@
    +    controller: '\Drupal\terms_of_use\AutocompleteController::autocomplete1'
    

    What is the 1 here for?

gvso’s picture

Status: Needs work » Needs review
FileSize
29.32 KB
30.87 KB

Hi! Here is my patch again. I didn't work in the number 9, because the patch doesn't work with node title yet and I cannot test if the code is working. Actually that part of the code is commented.

+++ b/src/Form/TermsOfUseForm.php
@@ -0,0 +1,186 @@
+      $nid = db_select('node', 'n')

Thank you for reviewing and just let me know if something is wrong...

larowlan’s picture

Some minor changes left - ignore the UUID comment - nid is what is in the D7 version - but UUID would be the ultimate goal - but is outside the scope of the GCI task in my opinion.

  1. +++ b/CHANGELOG.txt
    @@ -0,0 +1,30 @@
    +  ¶
    ...
    +  ¶
    

    nitpick: whitespace here

  2. +++ b/src/Form/TermsOfUseForm.php
    @@ -10,9 +10,12 @@
    +  private $node_title;
    

    Please document, and use protected (Drupal avoids private unless absolutely necessary)

  3. +++ b/terms_of_use.module
    @@ -21,25 +27,25 @@
    +    // Getting the node through its nid.
    +    $node = Node::load($terms_of_use_node_id);
         $translation = \Drupal::entityManager()->getTranslationFromContext($node, $langcode);
     
         // If we were able to load the node.
    @@ -47,13 +53,12 @@
    
    @@ -47,13 +53,12 @@
           // If we find @link in the text for the terms checkbox we just show a
           // link. Otherwise we show the full terms.
           if (strpos($checkbox_label, '@link') !== FALSE) {
    -        $checkbox_label = str_replace('@link', l($node->title, 'node/' . $node->nid), $checkbox_label);
    

    This hunk should probably be in an if statement - just in case the stored node ID no longer exists - especially if config is being deployed - the node won't yet exist?
    Larger question - we should be using $node->uuid() not node->id() because id() could change from site to site, whereas UUID would be consistent with a content deployment strategy (yay for new cool stuff in D8)

  4. +++ b/config/schema/terms_of_use.schema.yml
    @@ -0,0 +1,19 @@
    +      label: 'Terms of use node title' ¶
    

    Extra whitespace

gvso’s picture

Thank you very much again. Here is my patch. I didn't understand your point in the number 3. First it checks if a nid is specified in the configuration, next it checks if the nid exists yet and it wasn't deleted.

I realized that there wasn't white spaces in the .schema.yml, I think is about the new line at the end. The same happens in the CHANGELOG, but there I though you meant the two unnecessary new lines in the file. Thanks so much again.

larowlan’s picture

  1. +++ b/src/Form/TermsOfUseForm.php
    @@ -14,7 +14,13 @@
    +  * Save the node title in validateForm method
    +  * and next submitForm method uses it to save it
    +  * in the configuration management
    

    nitpick: This comment should go above the @var line and be less than 80 chars

  2. +++ b/terms_of_use.module
    @@ -45,15 +44,15 @@
    -    if ($node->nid) {
    +    if ($node->uuid) {
    

    Revert this change - switching to uuid is more involved than just this :)

gvso’s picture

Here is the patch with your suggestions... thanks!

larowlan’s picture

I think this is about ready to go

  • Kars-T committed e2dbedd on 8.x-1.x
    Issue #2388475 by gvso: Terms of Use version for Drupal 8
    
    Initial...
Kars-T’s picture

Dear gvso and larowlan.

To get this going I just pushed up a D8 branch with the patch. I had problems merging the .module file so I did this by hand and hope it is not missing something.

Is using "$node->nid" still correct? PHPStorm tells me it can't see this in the base class?

I will further review this. Thanks again!

gvso’s picture

Issue tags: +Needs reroll

Thanks @Kars-T! It seems that it needs a re-roll. I will work on it this weekend

Kars-T’s picture

@gvso If you promise me you won't break it you can have git access ;)
I finally have the access to do this.

gvso’s picture

Thanks @Kars-T, but I'm a little busy now so I don't intend to co-maintain the module in this moment, I would like to help you in another instance though

gvso’s picture

I have been working to reroll the module and I also want to update the test, but it's not working yet due to an issue with WebTestBase::drupalCreateNode(). So, I will add it as related.

ivnish’s picture

Status: Needs review » Fixed

Already ported

Status: Fixed » Closed (fixed)

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