Add an ALTCHA form field to the form generator #7054
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
leofeyer
changed the title
leofeyer
changed the title
leofeyer
changed the title
leofeyer
changed the title
Toflar
added
the
up for discussion
leofeyer
removed
the
up for discussion
Toflar
requested changes
Apr 4, 2024
There was a problem hiding this comment.
Hey Marko, first of all, thank you so much for taking the time to work on this ❤️ We have discussed this in today's call and here is a first feedback.
Also some general info:
- @leofeyer will prepare a component for your altcha.js
- We don't need the
Powprefix/namespace you have added everywhere, you can just omit that :) - Translations: You don't have to worry about them, we only manage the English translations and then synchronize translations using transifex. So the current state of the PR is perfect 👍
- Regarding the unit tests: Do you want to give it a try and learn something new? For the
AltchaControllerand thePurgeExpiredAltchaChallengesCronthings should be fairly straightforwad because we already have other controllers and cronjobs where you could check how the unit tests are written. This would probably be a good start? For theAltchaservice itself, we can also help 😊 (we can also for the rest, just wanted to check if you want to learn something new first 😎 )
Toflar
reviewed
Apr 4, 2024
Toflar
requested changes
Apr 10, 2024
ausi
reviewed
Apr 10, 2024
fritzmg
reviewed
Apr 10, 2024
fritzmg
reviewed
Apr 10, 2024
Toflar
reviewed
Apr 11, 2024
| // The challenge expires in 1 hour (default) and it must be saved in the database | ||
| // to prevent replay attacks. | ||
| $expiryDate = date('Y-m-d\\TH:i:sP', time() + $this->altchaChallengeExpiry); | ||
| $objAltcha = new AltchaEntity($challenge, new \DateTimeImmutable($expiryDate)); |
There was a problem hiding this comment.
I think this cannot work because that class does not exist (that's why we need tests ;)).
markocupic
commented
Apr 11, 2024
|
The script is now available as Contao component: https://packagist.org/packages/contao-components/altcha |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
As agreed with @Toflar on Slack, this PR delivers the integration of Altcha https://altcha.org/ for Contao 5.x
What are the thinks still needs to be done?
One point that is still open is the lack of unit tests, because I have too little experience in writing unit tests.
The translations are also missing.
The Altcha script is currently in the public directory and probably needs to be moved.
Please also take a close look at the widget class \Contao\AltchaForm. I am not sure if everything is 100% correct.
@ContaoCoreTeam Have a look at it and I look forward to constructive feedback 🌞