CVS edit link for zhangtaihao

As part of a larger web project, I plan to contribute these modules:
- UUID resolver: resolves UUID under a system path just like node ID under a node system path 'node/%'.
- Feeds delete: allows deletion of objects in website using some mechanism in Feeds. This shall initially be facilitated via extra mappings (may expand to a full blown feed processor if sufficiently different from existing feed processors).
- Feeds UUID targets: adds new UUID-based targets and handlers to feeds processors. Initially, the targets will be taxonomy and nodereference fields.
- Feeds subset import: allows filtering of feeds to be imported per importer. This is partially in response to http://drupal.org/node/856136 in addition to our own concern regarding a feed type per content type to import for. I plan to use Views for filtering so that I don't have to implement my own filtering mechanism.
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

zhangtaihao’s picture

Status: Postponed (maintainer needs more info) » Needs review

UUID resolver is here. See attachment.

Also, I've abandoned the "Feeds subset import" idea. At the moment, "Data Feeds" is more like it. It integrates with Feeds, Data, Views, Rules, etc. to facilitate a flexible feeds import framework without polluting node types. I am still debating which modules exactly to require and which ones to support.

The other modules are coming. I was wondering whether you could allow me to get going with at least this module.

zhangtaihao’s picture

For some reason, the last attachment didn't upload. I'm trying again.

pcambra’s picture

Status: Needs review » Needs work

Thanks for submitting your CVS Application!

Could you please include a more extense description of what UUID resolver does?
From CVS Application requirements: http://drupal.org/cvs-application/requirements

You must provide a motivation message, which should be a few paragraphs instead of only a few sentences. It should include details about your contribution, its features, how it compares to already existing solutions, links to screenshots or a demo, etc.

zhangtaihao’s picture

UUID resolver

The UUID resolver is a module that accepts a UUID under a registered wildcard menu path and redirects to the target object (i.e. node/term/etc.) the UUID identifies. It integrates with the UUID module to look up an object. When a page access hits the wildcard path with a UUID recognized in the system, the URL is redirected to the path of the target object.

For example, in a Drupal site http://www.example.com, node UUID lookup is registered on the path uuid/%. The site has a node:

  • nid: 2
  • uuid: 286acd2e-cd1d-11df-a386-005056812593

The following URL:

http://www.example.com/uuid/286acd2e-cd1d-11df-a386-005056812593

will be redirected to

http://www.example.com/node/2

Motivation

When publishing nodes as syndicated objects on a Drupal site (e.g. with Feeds), it normally has a canonical URL. The path of this URL is generally under node/XXXX. In most cases, the node ID doesn't change, so this URL remains "canonical" for the duration of the site.

However, in a massive network of websites sharing numerous nodes, migration of a domain from an existing Drupal platform to a new one (e.g. website renovation) can become prohibitively expensive and difficult to coordinate, requiring the cascaded updates of the existing canonical URLs (containing node IDs) to new ones, even before the new URL is set up. The natural solution would be to use a common identifier in the URL so that the URL would be truly canonical.

Since UUIDs are by definition universally unique, it would make sense to use it as a canonical identifier in a URL. This way, whichever installation a node is on, the same node should have only one UUID. This module then assists in redirecting that canonical URL to wherever the node is locally located. The domain update can then naturally progress without forcibly switching on maintenance mode for the potentially extended duration of a multi-tiered migration.

zhangtaihao’s picture

Status: Needs work » Needs review
KarenS’s picture

This is an interesting concept. We're doing a bunch of work around UUID to be able to migrate and deploy content across sites and maintain things like references. I don't know if this project would solve my particular problem or not, but it looks like an interesting solution.

The code looks nice and clean, haven't actually tried it out, just looked.

zhangtaihao’s picture

Well actually a few of us at the workplace have been banging our heads on the issue for the past two months. The primary problem through numerous attempts we failed to avoid is just the general lack of feasible core/third-party solutions to work using a distributed base data model. I suspect a lot of these problems will go away once D7 is able to fully utilize RDF and related technologies as its chief data interchange protocol. Then, we'd be able to plug SPARQL into everything!

On another note, we had almost succeeded in a distributed data architecture using Feeds when we ran into the need to #917678: Refactor code to use arbitrary types of feeds source in addition to nodes. We're almost definitely going dark with D6 until the possible backport, but I'd keep an eye on what's going on next there for D7. (Psst, I'm in the process of forking Feeds 6.x-1.x to do all of that just as a proof of concept to ourselves. I'll let you know when I make some headway.)

Then again, maybe this would suffice: http://github.com/kswan/super_nodereference

apaderno’s picture

Status: Needs review » Needs work
  • The points reported in this review are not in order of importance / relevance.
  • Most of the times I report the code that present an issue. In such cases, the same error can be present in other parts of the code; the fact I don't report the same issue more than once doesn't mean the same issue is not present in different places.
  • Not all the reported points are application blockers; some of the points I report are simple suggestions to who applies for a CVS account. For a list of what is considered a blocker for the application approval, see CVS applications review, what to expect. Keep in mind the list is still under construction, and can be changed to adapt it to what has been found out during code review, or to make the list clearer to who applies for a CVS account.
  1. License files cannot be committed in Drupal.org repository. Projects committed in Drupal.org repository have the same license used by Drupal.
  2. The version line needs to be removed from the .info file.
  3. The line endings should be the same used from Linux; the .info file is using different line endings.
  4. Drupal variables are not initialized.
  5. Deleting Drupal variables using a query that matches any Drupal variable with a name that starts with the module name would remove also the Drupal variables of other modules.
  6. Hook implementation comments should be like the following one:
    /**
     * Implements hook_menu().
     */
    

    As reported in Documenting hook implementations:

    If the implementation of a hook is rather standard and does not require more explanation than the hook reference provides, a shorthand documentation form may be used in place of the full function documentation block described above:

    /**
     * Implements hook_help().
     */
    function blog_help($section) {
      // ...
    }
    

    This generates a link to the hook reference, reminds the developer that this is a hook implementation, and avoids having to document parameters and return values that are the same for every implementation of the hook. Optionally, you can add more information in a separate paragraph to describe the particular quirks of your hook implementation.

    In the case of hooks that have variables in the names, such as hook_form_FORM_ID_alter(), a slightly expanded syntax should be used:

    /**
     * Implements hook_form_FORM_ID_alter() for node_type_form().
     */
    function mymodule_form_node_type_form_alter(&$form, &$form_state) {
      // ...
    }
    

    This generates a link to the hook reference, as well as to the particular form that is being altered. Again, optionally you can add more information in a separate paragraph to describe the particular quirks of your hook implementation.

  7.   $pattern = './' . drupal_get_path('module', 'uuid_resolver') . '/includes/*.inc';
      foreach (glob($pattern) as $inc) {
        include_once($inc);
      }
    

    Why isn't the code using file_scan_directory()

  8. Menu descriptions and titles, as well as schema descriptions, should not be passed to t().
  9. /**
     * Implementation of hook_load.
     * @return  Resolver info.
     */
    function uuid_resolver_info_load($name) {
      $resolvers = uuid_resolver_get_resolvers();
      return $resolvers[$name];
    }
    
    

    It's not an implementation of hook_load().

  10. It's not clear to me how the code handles URLs like http://www.example.com/uuid/286acd2e-cd1d-11df-a386-005056812593, when the module doesn't define a menu callback for uuid/%, or uuid/%uuid_resolver_id.
zhangtaihao’s picture

Status: Needs work » Needs review
FileSize
12.27 KB

The module has been cleaned up (in places I was able to spot anyway).

Regarding point 10 above, I have now added documentation to uuid_resolver_menu_alter() to indicate that it "Injects menu callbacks for each enabled resolver". In effect, resolvers register a base path (specified in admin UI) followed by /%, the callback for which trickles down to the resolve callback for a path to redirect to. Do you think this should be documented somewhere other than for uuid_resolver_menu_alter()?

zhangtaihao’s picture

Also, the guard for a resolver's base path can be found in a regular expression on line 354 of admin.inc.

I've attached a patch for all the changes this round for convenience and clarity.

zhangtaihao’s picture

I'd just like to find out if anyone is reviewing this at the moment.

rfay’s picture

Status: Needs review » Reviewed & tested by the community

This looks to me to be as good as anything anywhere in contrib.

zhangtaihao has been responsive and cooperative.

I think this application should be approved.

ilo’s picture

Status: Reviewed & tested by the community » Needs review

I'm going to make a review, but just want to point you that runing coder before submitting the module would help a lot, actually there are still some issues discovered by coder that require inmediate action:
- string concatenation.
- coding standard on comments.
- } else { statements.

Zhangtaihao, can you run coder and fix only the issues raised by the review? (so I can do manual review of other issues being sure that there is no important changes).

ilo’s picture

Status: Needs review » Reviewed & tested by the community

mm so, sorry, I just click edit before your submission, rfay. In fact, reviewing the code, apart from that noissy coder issues about string concatenation, everything looks fine. I agree with rfay and suggest to let this one go in, eventually I'll be using this module in the following days, so I'll be providing a testcase and some bug fixings, so an issue queue will help a lot to make this module go live for the 1.0 release.

rfay’s picture

ilo points out that uuid module is looking for co-maintainers. That would be a quick way in the door also. zhangtaihao, we welcome you here and think you can do great things in this area.

zhangtaihao’s picture

FileSize
10.02 KB

Thanks for helping to review the module! Actually, I was going to get xtfer to put the project on CVS. He got busy and this application moved forward. In any case I can now get some momentum on this project.

I've checked the code against both the latest stable and dev releases of Coder and have fixed all the problems it could expose. Annoyingly, I've always used to put a space before and after a concatenation operator. It's only after looking over many modules over in contrib that I've got the impression of there being no real rule about it. I've now pushed it to the github repo (if you're interested it's at http://github.com/anu-science/uuid_resolver). I've decided to leave this issue clean so this could move forward, but here's the patch anyway.

If there's anything else I need to do, let me know.

zhangtaihao’s picture

Sorry to pollute this issue again, but I found a bug and resolved it. So, to test the module, here's the latest patch in the serial.

Michelle’s picture

Status: Reviewed & tested by the community » Needs review

Not sure if this is still RTBC. There's a couple of patches added after it was marked so. Setting this back to needs review. If one of the reviewers could check if it should be set back to RTBC, that would be great.

Michelle

rfay’s picture

Status: Needs review » Reviewed & tested by the community

IMO this should be a go. It's not the code that we're RTBC'ing, it's the CVS application. zhangtaihao has well proven worth of the CVS account.

Michelle’s picture

Status: Reviewed & tested by the community » Fixed

Well, then, there you go. :)

Michelle

zhangtaihao’s picture

Thank you very much!

zhangtaihao’s picture

Status: Fixed » Closed (fixed)
ilo’s picture

Thank you all. I'll keep an eye on the project from now on.

rfay’s picture

@zhangtaihou, Congratulations! Looking forward to your contributions.

BTW: We just leave issues in "Fixed" so that people can find them. Drupal.org automatically closes them in 2 weeks if there's no further activity. So you don't have to change the issue to closed (fixed).

apaderno’s picture

Status: Closed (fixed) » Fixed
zhangtaihao’s picture

Alright, I'll keep that in mind in the future.

For reference, the project is located at:

http://drupal.org/project/uuid_resolver

Status: Fixed » Closed (fixed)

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