Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
This issue is to track progress on implementation in the frontend.
Comment | File | Size | Author |
---|---|---|---|
#11 | 0001-Fix-for-IDN-in-clone-migrate.patch | 10.47 KB | omega8cc |
#10 | 0001-Fix-for-IDN-aliases.patch | 1.8 KB | omega8cc |
#2 | 0001-IDN-domain-names-on-the-fly-conversion.patch | 10.79 KB | omega8cc |
Comments
Comment #2
omega8cc CreditAttribution: omega8cc commentedPatch for review attached.
Comment #3
ergonlogicDoes this functionality depend on https://www.drupal.org/project/idna_convert? Maybe we should add that to the makefiles?... Looking a little closer, this module appears to be a very thin wrapper around some library classes, but they're only included via composer.json. This could present some challenges.
One option would be to implement the Add a 'make-composer' command feature request in Drush.
Another would be to fork idna_convert, and commit the autoload.inc and vendor dirs into Git. In fact, this is close enough to Aegir's core functionality, that I think a hosting_idna module/feature would be reasonable to include in core.
As for the patch itself, all those calls to
function_exists()
seems like a code smell to me. Maybe what we really need aredrupal_alter()
s instead? Then hosting_idna could just implement the hook. This should allow us to replace each instance of something like:with:
Then, in hosting_idna.module:
Comment #4
ergonlogicWould
hook_hosting_process_url()
be too generic? Maybehook_hosting_encode_url()
instead?Comment #5
omega8cc CreditAttribution: omega8cc commentedYes, the patch for hostmaster is in the other queue: https://www.drupal.org/node/1899838#comment-11117603
We could of course implement this without idna_convert wrapper.
Comment #6
omega8cc CreditAttribution: omega8cc commentedWe have added it in our makefile using archive download from d.o and then "copy" option in Drush 8 (equivalent of make_local), so i didn't spot the problem with the library.
Comment #7
omega8cc CreditAttribution: omega8cc commentedHmm, but it seems that the lib is included.
It doesn't need anything else to work properly.
Comment #8
ergonlogicOh, does d.o packaging include components in composer.json now? Nevermind that bit then.
Comment #9
ergonlogicAh, I see. I'd missed that.
Comment #10
omega8cc CreditAttribution: omega8cc commentedThis will need a clean rewrite anyway, but if anyone wants to test this further, here is another patch to be applied after the initial patch.
Comment #11
omega8cc CreditAttribution: omega8cc commentedThis will need a clean rewrite anyway, but if anyone wants to test this further, here is another patch to be applied after the two previous patches.
Comment #12
omega8cc CreditAttribution: omega8cc commentedOf course we could avoid all those if/else checks/alternatives if we will force the IDN code/submodule as always enabled and active (in core or contrib).
I believe that it should be just always active and included in core, and we could clean it up further via universal 'sanitize' function, so we don't need to repeat also the
strtolower(trim(foo))
part everywhere.Comment #13
ergonlogicI created a new issue: #2712517: Improve URL handling, to add a
hook_hosting_url_alter()
hook (note that I'd gotten thedrupal_alter()
syntax wrong above.) I still believe that adding a hook is the best way forward, and would drastically simplify implementing IDNA support.While I agree that this should probably be enabled by default, I believe it would constitute an API change, which would mean waiting for an Aegir 4.x release. Adding a hook while otherwise keeping behaviour consistent, on the other hand, is an API addition, which wouldn't require a major version bump. Implementing this as a separate, if very small, module, would give us the flexibility of including it in core, as an 'experimental' feature, pretty much right away.
Comment #14
helmo CreditAttribution: helmo at Initfour websolutions for Aegir Cooperative commented+1 for improving this and enabling by default for new installations if it's tested a bit.
The drupal_alter seems like the most flexible way...