This issue is to track progress on implementation in the frontend.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

omega8cc created an issue. See original summary.

omega8cc’s picture

Assigned: omega8cc » Unassigned
Status: Active » Needs review
FileSize
10.79 KB

Patch for review attached.

ergonlogic’s picture

Status: Needs review » Needs work

Does 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 are drupal_alter()s instead? Then hosting_idna could just implement the hook. This should allow us to replace each instance of something like:

if (function_exists('idna_convert_encode')) {
  $url = idna_convert_encode($url);
}

with:

$url = drupal_alter('hosting_process_url', $url);

Then, in hosting_idna.module:

/**
 * Implements hook_hosting_process_url().
 */
hosting_idna_hosting_process_url($url) {
  $IDN = new idna_convert();
  return $IDN->encode($domain);
}
ergonlogic’s picture

Would hook_hosting_process_url() be too generic? Maybe hook_hosting_encode_url() instead?

omega8cc’s picture

Yes, 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.

omega8cc’s picture

We 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.

omega8cc’s picture

Hmm, but it seems that the lib is included.

It doesn't need anything else to work properly.

ergonlogic’s picture

Oh, does d.o packaging include components in composer.json now? Nevermind that bit then.

ergonlogic’s picture

Ah, I see. I'd missed that.

omega8cc’s picture

This 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.

omega8cc’s picture

This 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.

omega8cc’s picture

Of 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.

ergonlogic’s picture

I created a new issue: #2712517: Improve URL handling, to add a hook_hosting_url_alter() hook (note that I'd gotten the drupal_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.

helmo’s picture

+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...