Problem/Motivation

No script element

<noscript>
<style>form.antibot * :not(.antibot-message) { display: none !important; }</style>
</noscript>

is previewed on all pages, but we need it only on pages with antibot protected forms.

CommentFileSizeAuthor
#17 functionalJavascript.png26.34 KBTauany Bueno
#17 functionalTests.png64.49 KBTauany Bueno
#17 message_after.png65.49 KBTauany Bueno
#7 3195229_after.png176.58 KBhmendes
#7 3195229_before.png179.2 KBhmendes

Issue fork antibot-3195229

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

sakiland created an issue. See original summary.

sakiland opened merge request !1

sakiland’s picture

sakiland CreditAttribution: sakiland as a volunteer commented
Priority: Normal » Minor
Issue tags: +Needs tests

We need to write an automated test to cover this functionality.

gaurav.kapoor’s picture

gaurav.kapoor CreditAttribution: gaurav.kapoor at Axelerant for Drupal India Association commented
Priority: Minor » Normal

paulocs made their first commit to this issue’s fork.

paulocs’s picture

paulocs
Primary language Portuguese, Brazil
Location Belo Horizonte
CreditAttribution: paulocs at CI&T commented
Status: Active » Needs review
Issue tags: -Needs tests

Added tests to it.

hmendes’s picture

hmendes CreditAttribution: hmendes at CI&T commented
Status: Needs review » Reviewed & tested by the community
FileSize
3195229_before.png179.2 KB
3195229_after.png176.58 KB

Tested the MR and it worked perfectly, attaching prints from before and after the MR.

gaurav.kapoor’s picture

gaurav.kapoor CreditAttribution: gaurav.kapoor at Axelerant for Drupal India Association commented
Status: Reviewed & tested by the community » Needs work

This is working for me, but the issue is the usage of the drupal_static function which will be marked deprecated soon (Check https://www.drupal.org/project/drupal/issues/2260187) and wouldn't exist in Drupal 10. Let's try to resolve it in a different way. Thanks for adding the test though.

sakiland’s picture

sakiland CreditAttribution: sakiland as a volunteer commented

Thank you @hmendes, @paulocs, @gaurav.kapoor for the help. I will try to find some better solution for this.

sakiland’s picture

sakiland CreditAttribution: sakiland as a volunteer commented
Status: Needs work » Needs review

I've removed usage of the drupal_static method, and set variable as global.

All tests for Drupal 8 passed, but there are 2 failed tests for Drupal 9.2 (ignore test suite run on PHP 7.3 & MariaDB 10.2.7 D9.2, I've chosen unsupported MariaDB version for Drupal 9, my mistake)

I cannot discover why 2 tests failed for Drupal 9, any help would be great. Thank you!

gaurav.kapoor’s picture

gaurav.kapoor CreditAttribution: gaurav.kapoor at Axelerant for Drupal India Association commented
Status: Needs review » Needs work

I am not sure if using global is also the right solution here. Marking needs work to get more feedback around it and check why tests are failing.

beatrizrodrigues’s picture

beatrizrodrigues
She/Her
CreditAttribution: beatrizrodrigues at CI&T commented
Assigned: Unassigned » beatrizrodrigues

I will check why the tests are failing

gaurav.kapoor’s picture

gaurav.kapoor CreditAttribution: gaurav.kapoor at Axelerant for Drupal India Association commented

Thanks, @beatrizrodrigues. Do you have any feedback regarding the implementation by @sakiland in #9?

beatrizrodrigues’s picture

beatrizrodrigues
She/Her
CreditAttribution: beatrizrodrigues at CI&T commented
Assigned: beatrizrodrigues » Unassigned

@gaurav.kapoor I'm sorry, but I got really stuck at this issue. I spent a lot of time local testing the classes that was returning errors here, but i couldn't be able to understand why they are failing. Locally, the AntibotNoJavascriptTest and AntibotCacheTagsTest tests are returning "OK", only the AntibotJavascriptTest tests has a bad return. (I'm testing all in D9.2). It returns me:

Behat\Mink\Exception\ResponseTextException: The text "xdwdw8w3 is not recognized as a username or an email address." was not found anywhere in the text of the current page.

I'm new here so I'm still learning how to deal with some problems I face it. So, I couldn't tell if it is a actual error or something is missing locally here. I wish I could solve this. I'm open to learn if anyone wants to share something about this issue.

gaurav.kapoor’s picture

gaurav.kapoor CreditAttribution: gaurav.kapoor at Axelerant for Drupal India Association commented
Version: 8.x-1.x-dev » 2.0.x-dev
Tauany Bueno’s picture

Tauany Bueno CreditAttribution: Tauany Bueno at CI&T commented
Assigned: Unassigned » Tauany Bueno

hi! i'll work on it :)

Tauany Bueno’s picture

Tauany Bueno CreditAttribution: Tauany Bueno at CI&T commented
Status: Needs work » Needs review
FileSize
message_after.png65.49 KB
functionalTests.png64.49 KB
functionalJavascript.png26.34 KB

I fixed some PHPCS errors, and also checked the tests and removed some deprecated functions. All functional tests were passing on my local environment, I'm just not really sure about the FunctionalJavascript (I'll attach some images). I'll leave the status as needs review, but i'd be happy to help fix any other problems that may appear.

tauanygb opened merge request !10

Tauany Bueno’s picture

Tauany Bueno CreditAttribution: Tauany Bueno at CI&T commented
Assigned: Tauany Bueno » Unassigned

Hello!
I ended up committing to the wrong version of the module, so I made a new merge request to the right version (2.0.x0-dev) fixed some conflicts. All functional tests were passing on my local environment, I'm just not really sure about the FunctionalJavascript (see attached images). I'm leaving the status as needs review, I'm new to Drupal, so I'm still learning how to solve some problems, but I'd be more than happy to learn and help fix any other issue that might appear.

beatrizramos made their first commit to this issue’s fork.

beatrizramos’s picture

beatrizramos CreditAttribution: beatrizramos at CI&T commented
  1. I tested the FunctionalJavascript test and it's working properly,
  2. I made a commit that fixes the phpcs errors that were showing.
damiaosj’s picture

damiaosj CreditAttribution: damiaosj at CI&T commented
Assigned: Unassigned » damiaosj

Hi! I'll review this one.

damiaosj’s picture

damiaosj CreditAttribution: damiaosj at CI&T commented
Assigned: damiaosj » Unassigned
Status: Needs review » Reviewed & tested by the community

Hi! I have made the review and talked directly with @beatrizramos to solve just one error that was showing in the PHPCS. We fixed it, and reviewed again and now everything is fine!

Due there's no errors or warnings, I'm changing the status to RTBC.

gaurav.kapoor’s picture

gaurav.kapoor CreditAttribution: gaurav.kapoor at Axelerant for Drupal India Association commented

Looking into this, let's try to get tests working as well.

gaurav.kapoor’s picture

gaurav.kapoor CreditAttribution: gaurav.kapoor at Axelerant for Drupal India Association commented
Status: Reviewed & tested by the community » Needs work

Few conversation still open in MR.

vishaljd’s picture

vishaljd CreditAttribution: vishaljd at Valuebound commented
Assigned: Unassigned » vishaljd

vishaljd opened merge request !11

vishaljd’s picture

vishaljd CreditAttribution: vishaljd at Valuebound commented
Assigned: vishaljd » Unassigned
Status: Needs work » Needs review
paulocs’s picture

paulocs
Primary language Portuguese, Brazil
Location Belo Horizonte
CreditAttribution: paulocs at CI&T commented
Assigned: Unassigned » paulocs
Status: Needs review » Needs work

Instead of adding the library via hook_page_attachments, we can add it via JS but it will not allow other modules to edit the antibot style tag in the head element using the hook_page_attachments_alter. I think its the only way to avoid the noscript element to be rendered.

The hook_page_attachments is called before the hook_form_alter, so the way it's now, the noscript element is never rendered because the $_antibot_protection variable will be always NULL (unitialized).

We should evaluate if we add the noscript tag when the page is loaded via JS or if we keep the way it is (loading on all pages).

I'll address the comments added the the merge request 10.

MR11 doesn't have the changes to fix this issue. It has only code standard fix. We can keep our effort working on the branch of MR 10 (3195229-no-script-element).

paulocs’s picture

paulocs
Primary language Portuguese, Brazil
Location Belo Horizonte
CreditAttribution: paulocs at CI&T commented
Assigned: paulocs » Unassigned

Keep NW because of #29.