Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
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.
Comment | File | Size | Author |
---|---|---|---|
#10 | interdiff-2388475-8-10.txt | 1.29 KB | gvso |
#10 | terms_of_use-port-to-drupal-8-2388475-10.patch | 29.36 KB | gvso |
#8 | interdiff-2388475-6-8.txt | 2.26 KB | gvso |
#8 | terms_of_use-port-to-drupal-8-2388475-8.patch | 29.44 KB | gvso |
#6 | interdiff-2388475-2-6.txt | 30.87 KB | gvso |
Comments
Comment #1
gvsoHere 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.
Comment #2
gvsoI forgot about this. This patch works correctly, but only enter a nid is valid..
Comment #3
gvsoComment #4
gvsoComment #5
larowlanPlease add this file back - and add an entry for your work :)
This should not be in the repo, d.o packager auto-adds it, please remove
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
Needs a newline on the last line
Don't pass config values through $this->t(), add a schema and they can be translated via config_translation module
Extra space here after (
use Node::load instead
$node->label() now, $node->title is a FieldItemListInterface object, not a scalar
You can inject the db connection and use $this->database->select() instead of the procedural wrapper
form_set_error is gone too, use $form_state->setErrorByName()
variable_set is gone - this should be replaced with config now
These two aren't present in the default settings.yml file, please add them
please don't use $_POST direct, use $form_state->getValue()
add a use statement and use the alias in the argument signature instead
add a use statement and use the Language alias instead
Add a use statement and use the XSS short-hand alias instead
nitpick: needs a trailing .
Node::load now
$translation->body->processed should be used instead
remove this
What is the 1 here for?
Comment #6
gvsoHi! 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.
Thank you for reviewing and just let me know if something is wrong...
Comment #7
larowlanSome 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.
nitpick: whitespace here
Please document, and use protected (Drupal avoids private unless absolutely necessary)
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)
Extra whitespace
Comment #8
gvsoThank 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.
Comment #9
larowlannitpick: This comment should go above the @var line and be less than 80 chars
Revert this change - switching to uuid is more involved than just this :)
Comment #10
gvsoHere is the patch with your suggestions... thanks!
Comment #11
larowlanI think this is about ready to go
Comment #13
Kars-T CreditAttribution: Kars-T commentedDear 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!
Comment #14
gvsoThanks @Kars-T! It seems that it needs a re-roll. I will work on it this weekend
Comment #15
Kars-T CreditAttribution: Kars-T commented@gvso If you promise me you won't break it you can have git access ;)
I finally have the access to do this.
Comment #16
gvsoThanks @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
Comment #17
gvsoI 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.
Comment #18
ivnish CreditAttribution: ivnish commentedAlready ported