Problem/Motivation

Not all sites use node, but may want CAPTCHA for other purposes. Currently, there is a hard dependency on the node module.

Proposed resolution

It would be better to wrap this functionality in a check that node is enabled.

User interface changes

None if node is enabled.

API changes

Can enable without node.

CommentFileSizeAuthor
#20 interdiff.txt488 bytesrlmumford
#20 2959854-20.patch3.34 KBrlmumford
#17 removed-dependency-on-node-2959854-17.patch3.21 KBtanmayk
#11 removenodedependency-2959854-11.patch287.65 KBelachlan
#8 removenodedependency-2959854-6.patch273 bytesvakulrai
#7 removenodedependency-2959854-6.patch273 bytesvakulrai
#5 removenodedependency-2959854-5.patch285.71 KBvakulrai
#2 2959854-2-remove_node_dependency.patch2.49 KBandrewbelcher

Comments

andrewbelcher created an issue. See original summary.

andrewbelcher’s picture

andrewbelcher commented
Status: Active » Needs work
StatusFileSize
new 2959854-2-remove_node_dependency.patch2.49 KB

Here's a patch that basically does it, which demonstrates how unnecessary the dependency is.

This still needs work, as for the change in captcha_help I have simply prevented an error, but the wording probably needs adjusting, but I'm not sure I'm the right person to do that!

elachlan’s picture

elachlan commented
Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 2: 2959854-2-remove_node_dependency.patch, failed testing. View results

vakulrai’s picture

vakulrai commented
Status: Needs work » Needs review
StatusFileSize
new removenodedependency-2959854-5.patch285.71 KB

@ andrewbelcher I am adding a patch which will remove dependency globally.

Status: Needs review » Needs work

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

vakulrai’s picture

vakulrai commented
StatusFileSize
new removenodedependency-2959854-6.patch273 bytes

Applying a patch which removes dependency globally.

vakulrai’s picture

vakulrai commented
Status: Needs work » Needs review
StatusFileSize
new removenodedependency-2959854-6.patch273 bytes

Applying a patch which removes dependency globally.

Status: Needs review » Needs work

The last submitted patch, 8: removenodedependency-2959854-6.patch, failed testing. View results

elachlan’s picture

elachlan commented

You need to do it all in one patch.

elachlan’s picture

elachlan commented
Status: Needs work » Needs review
StatusFileSize
new removenodedependency-2959854-11.patch287.65 KB

Status: Needs review » Needs work

The last submitted patch, 11: removenodedependency-2959854-11.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

elachlan’s picture

elachlan commented

@vakulrai most of these are code style changes. Maybe we should make those changes in #2887225: Coding Standard issues and keep these changes specific to the issue?

@andrewbelcher - If you can re-roll your patch i'll add it in.

prashant.c’s picture

prashant.c
he/him
Primary language English
Location Dharamshala
commented

@elachlan

This issue is specifically about removing dependency of node module which is what #2 has done.

In your patch i can only see indentation so ideally your patch for indentation should go in #2887225: Coding Standard issues

elachlan’s picture

elachlan commented

@Prashant.c

Thanks for your input on the issue.

I was re-rolling the changes vakulrai had made in two separate patches.

I did this before I was aware vakulrai had made code style changes.

couturier’s picture

couturier commented
Related issues: +#2982140: Captcha is unusable with current beta and DEV state

We're really needing some help with the port to version 8.x so we can get this and other patches applied and a stable version released. Is anyone able to assist? See discussion at attached link.

tanmayk’s picture

tanmayk
he/him
Primary language English
Location Maharashtra
commented
Status: Needs work » Needs review
StatusFileSize
new removed-dependency-on-node-2959854-17.patch3.21 KB

Here is the rewrite of patch from #2 for current 8.x-1.x branch. I feel this should get committed soon.

Status: Needs review » Needs work

The last submitted patch, 17: removed-dependency-on-node-2959854-17.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

bradjones1’s picture

bradjones1
Primary language English
commented
Issue tags: +Needs tests

"I feel this should get committed soon." - That's all relative, given this still has test failures. This remains needs work.

My read on the test failures is that node now needs to be added to the $modules array on the best base class.

rlmumford’s picture

rlmumford
Primary language English
Location Manchester
commented
Status: Needs work » Needs review
StatusFileSize
new 2959854-20.patch3.34 KB
new interdiff.txt488 bytes
rlmumford’s picture

rlmumford
Primary language English
Location Manchester
commented
Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs tests

  • japerry committed 689e832 on 8.x-1.x authored by andrewbelcher 
    Issue #2959854 by vakulrai, rlmumford, elachlan, andrewbelcher, tanmayk...
japerry’s picture

japerry
Location KOMK
commented
Status: Reviewed & tested by the community » Fixed

Status: Fixed » Closed (fixed)

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