Support from Acquia helps fund testing for Drupal Acquia logo

Comments

jamesdixon created an issue. See original summary.

jamesdixon’s picture

Status: Active » Needs review
FileSize
2.46 KB

I've created a draft o the HTML Entity Decode plugin. Thanks for having a look.

jamesdixon’s picture

Again, cleaning these plugins up based on @ericgsmith's earlier feedback. This makes them much simpler.

ericgsmith’s picture

Status: Needs review » Needs work

Thanks for this too - just some very minor changes - I'm happy to update the test to extend the new base class if you like.

  1. +++ b/src/Plugin/Tamper/HTMLEntityDecode.php
    @@ -0,0 +1,33 @@
    +use Drupal\Core\Form\FormStateInterface;
    

    Unused, can be removed.

  2. +++ b/src/Plugin/Tamper/HTMLEntityDecode.php
    @@ -0,0 +1,33 @@
    + *   description = @Translation("Convert all HTML entities such as &
    

    As per the string to time review - I believe this can be one line.

  3. +++ b/src/Plugin/Tamper/HTMLEntityDecode.php
    @@ -0,0 +1,33 @@
    +class HTMLEntityDecode extends TamperBase {
    

    Code standard for acronyms is to use CamelCase e.g. HtmlEntityDecode

jamesdixon’s picture

Thanks for the feedback I've made these updates.

jamesdixon’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 5: htmlentitydecode-2960830-5.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

jamesdixon’s picture

Was missing a dependency in the last patch. Here's one that works!

ericgsmith’s picture

@James the new patches look like there were all enhancements to 2, not 3.

The changes in 3 were good - any reason to revert them?

jamesdixon’s picture

No sorry, I must've grabbed the wrong patch. I will reroll this patch with the stuff from 3 when I get a chance.

jamesdixon’s picture

Status: Needs work » Needs review
FileSize
2.1 KB
2.11 KB

Thanks for catching that @ericgsmith! I have added #3 back in, and also added the new Tamper Base Test class.

Cool, interdiff 7 11!

ericgsmith’s picture

Status: Needs review » Needs work

Nice - nearly there with this one too :)

+++ b/tests/src/Unit/Plugin/Tamper/HtmlEntityDecodeTest.php
@@ -0,0 +1,31 @@
+    $plugin = new HtmlEntityDecode([], 'html_entity_decode', []);

As per the string to time review, you can use $this->plugin here rather than creating a new plugin.

Would also be good to test for the exception that is thrown.

jamesdixon’s picture

Sounds good. When testing for the exception, do you do a try catch statement assertment somehow, or just assert it is a string?

jamesdixon’s picture

Status: Needs work » Needs review
FileSize
2.52 KB
1.19 KB

I think I'm picking up what you're throwing down @ericgsmith. I think this patch nails that exception confirming assertion you're looking for in the test.

ericgsmith’s picture

Status: Needs review » Reviewed & tested by the community

Awesome stuff - this looks good to go

ericgsmith’s picture

Status: Reviewed & tested by the community » Fixed

Status: Fixed » Closed (fixed)

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