Currently 10.1 support only exists in the 2.x branch. The 2.x branch does not appear to be ready for use, with as I understand it still significant re-writes planned for its future.
In #3299305: Automated Drupal 10 compatibility fixes it was stated:
But there will never be a Drupal 10 version of the 1.x branch. There is an upgrade path, and it is simply impossible to provide support for D8 and D10 with the same code. So, the current code will support D8 and D9, and the new even more heavily refactored branch will support D9 and D10 (and already does).
I'm not sure this is the case, I have done a cursory review and a cursory patch and I notice no significant issues and I have passing tests.
N/A
Backport 10.1 changes to 8.x-1.x
Patch
None
None expected
None
| Comment | File | Size | Author |
|---|
Show commandsStart 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:
changes
Comments
Comment #3
cmlaraI've only given it about 60 seconds of lab testing so I certainly want to spend more time to make sure I haven't missed anything, however since it is passing all tests lets tag this as NR to get more eyes to be sure nothing has been missed, especially given the previous claims that "impossible to provide support for D8 and D10 with the same code."
Note: we don't have D8 to test on D.O. so this portion requires manual review..
Comment #4
gaddman commentedDone some basic testing and all OK:
Comment #5
damienmckennaI think a key thing here is to make sure it still works on D8 - anyone have a D8 site to test it on?
Comment #6
ambient.impact@DamienMcKenna Drupal 8? In this economy? I kid but couldn't resist.
I'm going to check this out and rebase it on the latest 8.x-1.x so that it has yesterday's security update, and then I'll give it a try on our Drupal 10.1 site because this nearly made me spit out my coffee when I saw the security advisory.
Comment #7
ambient.impactIn my initial test of this on a local copy of a Drupal 10.1 site, two factor seems to be working. Will get more people to test it and report in.
Comment #8
cmlaraEnsuring D8 is indeed a key part of this issues concern, as is ensuring we don't miss any D10 changes either.
D8 testing is ensuring the fixes provide appropriate BC to the older releases and are more narrow in scope (we only need to test the code that has changed) while D10 is about ensuring that there are any other deprecation that were not picked up by automated tooling.
I couldn't find any major changes in the 2.x branch however specifically tagged to D10 so I'm expecting that to work well.
Comment #9
mingsongShould we update the line 11 in composer.json file
https://git.drupalcode.org/issue/tfa-3370841/-/blob/3370841-add-d10-supp...
From:
to:
As what the 2.x branch did since Encrypt module 3.0 is not D10 supported.
Comment #10
eelkeblokIf we don't actually need it in our code, I don't think we should. Other restrictions in a project will mean that Encrypt will in fact be at 3.1 if the project is on D10. All this is saying is that this project needs at least Encrypt 3.0 to run (which I'll have to assume is true, I don't actually know).
Comment #11
keshavv commentedBoth operators will do almost similar things. There is very little difference https://getcomposer.org/doc/articles/versions.md#tilde-version-range-
I think we can go with
^sign.MR Updated, Thank you.
Comment #12
cmlaraI hand tested the EntryForm on the MR as it existed at 2074c6a0 using D8.9.20 (I should have use older but realized I fired up a newer lab only after I did testing) and as we expected it still works providing the correct flood service and the flood service still returns errors when the configured flood timeouts are exceed.
If its not 100% necessary in order to allow TFA to work on D10 (while retaining support for D8) it would be out of scope for this issue.
Its helpful to keep patches clean and on target as much as possible.
Comment #13
mingsongUpload a patch from the MR for testing purpose.
Comment #14
mingsongSorry the patch #13 is misformatted.
Here is the new patch aiming for 8.x-1.x branch.
Comment #15
cmlara@Mingsong: It is not necessary to upload patches when a MR workflow is being used. If you need a patch for testing you may download it to your local environment following the instructions from https://www.drupal.org/docs/develop/git/using-gitlab-to-contribute-to-dr... without uploading to the issue.
I've updated the MR to revert 0ef241d0 per notes in #12.
Comment #17
cmlaraMerged into Dev.
Comment #18
mingsongHi @Conrad,
Thanks for merging it into the dev branch, that makes it easier for us to test it without requiring a patch.
As a MR could change anytime, we can't relying on a patch that directly comes from a merge URL, such as
https://git.drupalcode.org/project/tfa/-/merge_requests/26.patch
Comment #19
chrissnyderThank you for all of your work on this. I am excited to see a new release with this change so our site does not have to rely on the patch anymore, and the security advisory policy covers the code.
Comment #20
eelkeblokMy comment was more about the change from requiring version 3.0 to 3.1 than what operator is in use.
Comment #21
benjifisher@Mingsong:
Since the patch updates the version constraint, you will have to jump through some extra hoops if your Drupal version depends on that updated constraint. If you want to use a patch, then you can download a copy of the patch to your repository and use that. This is slightly more secure than downloading a patch from d.o.
You can also use the dev version of the module: something like
composer require drupal/tfa:1.x-dev.Comment #22
mingsongThanks @Benji for the advice.
Yes, I think we need to reconsider how to patch a Drupal module via using Composer.
Security-wise, Yes I think a 'local' solution would be more reliable.
Sharing-wise, a patch uploaded to Drupal.org is the directly way to shared with others in the Drupal community.