[D7] User Content Type

There are some errors reported by automated review tools, did you already check them? See http://pareview.sh/pareview/httpgitdrupalorgsandboxMartinSchauer2071679git

We are currently quite busy with all the project applications and we prefer projects with a review bonus. Please help reviewing and put yourself on the high priority list, then we will take a look at your project right away :-)

Also, you should get your friends, colleagues or other community members involved to review this application. Let them go through the review checklist and post a comment that sets this issue to "needs work" (they found some problems with the project) or "reviewed & tested by the community" (they found no major flaws).

I'm a robot and this is an automated message from Project Applications Scraper.

Hi,

first, the git link in your first post is your personal git link, please provide the public one:
git clone http://git.drupal.org/sandbox/MartinSchauer/2071679.git user_content_type

user_content_type.module
--------------------------------------

• You store many variables in the database because you use the system_settings_form(); Create a .install file and provide a hook_uninstall to delete the stored variables.
• line 157 and 369: in db_query the 'user' value should be a placeholder. Use the arguments array for this function.

the other code looks good.

Kind regards
kasalla

How this is different from user profiles?
I don't see any point on having user as nodes, as users can be managed via profiles.
Moreover, user profiles CAN be handled by Views!

@Martin sorry I don't see limitations on using users in Views. Can you explain it better?

For example: if you need a view listing users you can just create it by setting Show: Users on the View create form.

Another example: if you are listing content and you need extra information about the user, you can use Views relationships. Relating the content with the users will display all the user fields or any other information available on creating a view as in the first example.

@Martin I took a better look at your example and now I think I understand what you are trying to accomplish. In the website you sent they are listing different node types (not sure if it is really node types) as map layers. In addition, there is also users in this listing.

The problem there is listing different types of entities. By default when you create a view you need to choose a type, such as content, users, files, etc...

I don't think switching the user profiles to nodes is the best approach here. The reason is that later you may need to list other different entities and duplicating these entities as nodes would not be the best practice. Moreover, you will lose a lot of functionalities if you handle users as nodes, besides some problems it may bring later to your project.

The best thing to do it to research how to list different entity types in the same view. This way you will keep all good things that all these different entities offer. I've never done such thing, but I'd be interested on how this can be done.

@Martin, the solution you are proposing leads to more problems because if the admin chooses to use the user node he will have to manage the duplicated entries. Believe me, this is a big problem you don't want on your site.

You better solve it as proposed here: http://drupal.stackexchange.com/questions/25306/how-do-i-create-a-view-o...

The description of the issue is very similar to the example you had pointed.

I haven't searched for modules that can do it. If you cannot find I think you should change your strategy and do it in a generic way that other people in need to list different entities will also benefit from your project.

@mac_weber - I don't think your reviews so far have really followed the project application review guidelines. Your insight and feedback is helpful, but it would be easy to read comment #10 as an requirement rather than a suggestion and I think it should be a suggestion. In my opinion it's very reasonable to want a node per user. There may be reasons why this creates problems, but for certain sites I can see how it would be a reasonable solution. I don't see this as a performance/administrative/security issue. It's an architectural choice which is not relevant to project application process EXCEPT that the pros/cons of this approach should be documented.

@greggles you read my comments correctly. I really see this approach as a performance/administrative/security issue and also a bad practice.

Nodes are not designed to be used as users. You cannot do access control on user-nodes, for example. Having duplicates of real users and user-nodes leads to *many* administrative problems.
This way the system cannot get:

• User language
• User access control
• User password retrieval
• User profile links (duplicated with real user and user-nodes)
• Many many other things defined at $user and/or controlled using it

Any modules that depends of something about users rely on $user. Using such approach means these modules get broken in systems that implement user-nodes. My biggest concern is access control, but as I said, it is not the only critical thing affected here.

I had a chat with @greggles and he convinced me that this is not a performance/administrative/security problem as long it is explained how this module is intended to be used and in fact for some projects it may a feature, not a bug =)

However, there are other problems:
http://drupalcode.org/sandbox/MartinSchauer/2071679.git/blob/refs/heads/...

 $node->title .= ((isset($user->{$title_field_1}) && !empty($user->{$title_field_1}) && isset($user->{$title_field_1}[LANGUAGE_NONE][0]['value'])) ? $user->{$title_field_1}[LANGUAGE_NONE][0]['value'] : '');

There are other problems like this
Read: https://drupal.org/coding-standards#linelength

Control structure conditions should also NOT attempt to win the Most Compact Condition In Least Lines Of Code Award™

http://drupalcode.org/sandbox/MartinSchauer/2071679.git/blob/refs/heads/...

drupal_set_message(check_plain(t('Successfully added the content type "User" and @count user-nodes.', array('@count' => $user_add_count))));

Read: https://api.drupal.org/api/drupal/includes%21common.inc/function/t/6
(The docs for D7 for this function are not so good as for D6)
You should not run check_plain() on t(). Just using @variable is enough and t() will take care of it.

.
http://drupalcode.org/sandbox/MartinSchauer/2071679.git/blob/refs/heads/...
function _user_content_type_node_create() and many other functions need @param comments for its arguments.
Read: https://drupal.org/coding-standards/docs#param

The code on http://drupalcode.org/sandbox/MartinSchauer/2071679.git/blob/refs/heads/... should ideally use format_plural so that the noun can match the count of items (in case there is only 1 node created, for example).

@Martin, just a thought, are you using the location module for the actual coordinates? The location_entity module has views handlers to let you create a view of location entities directly. If you're location_cck to connect a location to a user, I don't think there is a built in relationship handler for that in the location module, but that wouldn't be that difficult to do as a contrib project.

Long term that might be a better solution to include any entity on your map, Redhen contacts, relations, taxonomy, etc.

// Content type exists, delete it.
if (isset($contenttypes['user_node'])) {
  // Delete all content.
  $user_nodes = node_load_multiple(array(), array('type' => 'user_node'));
  foreach ($user_nodes as $user_node) {
    node_delete($user_node->nid);
  }

Since you just need nid here, so node_load_multiple() would be an expensive operation. It would be better to use db_select() instead.

  $nids  = db_select('node', 'n')
    ->fields('n', array('nid'))
    ->condition('type', 'user_node')
    ->execute()
    ->fetchCol();
  node_delete_multiple($nids);

@Marting change the README to explain "to be able to handle them as nodes." It is misleading, as the user-nodes will be used only for listing the users, not to handle them. They should not be used, for example, to get the language of this user. I think this was one of the main reasons I was opposing to this module, as I was thinking on it as a way to really handle users.

Moving it to RTBC, as it's a minor change you can easily fix.

@Martin Schauer

There is a similar occurence of the code in .install file as well: Line #17 to Line #20. This can be changed as well.

@Marting read https://drupal.org/coding-standards#linelength

You can make conditionals look simpler at function _user_content_type_update_fields(): http://drupalcode.org/sandbox/MartinSchauer/2071679.git/blob/refs/heads/...

@Martin the problem is not how complex it is, but it is a very long line which you can shorten by using tokens as in the example:

The logic is the same simple logic, but do you see difference on the readability of the code? It helps other people to understand and contribute to your module in the future.

/**
 * DON'T DO THIS!
 */
  if ((isset($key) && !empty($user->uid) && $key == $user->uid) || (isset($user->cache) ? $user->cache : '') == ip_address() || isset($value) && $value >= time())) {
    ...
  }

/**
 * DO DO THIS!
 */

// Key is only valid if it matches the current user's ID, as otherwise other
  // users could access any user's things.
  $is_valid_user = (isset($key) && !empty($user->uid) && $key == $user->uid);

  // IP must match the cache to prevent session spoofing.
  $is_valid_cache = (isset($user->cache) ? $user->cache == ip_address() : FALSE);

  // Alternatively, if the request query parameter is in the future, then it
  // is always valid, because the galaxy will implode and collapse anyway.
  $is_valid_query = $is_valid_cache || (isset($value) && $value >= time());

  if ($is_valid_user || $is_valid_query) {
    ...
  }

Good job Martin!

Replaced the project's short-definition with a better one.

manual review:

1. the project page should also mention the profile2 module as similar module.
2. "'#empty_option' => '- No data -',": all user facing text must run through t() for translation. Also elsewhere like "'#value' => 'Delete user content',". Please check all your strings.
3. user_content_type_uninstall() duplicates _user_content_type_delete_all()? Why don't you call it from there?
4. _user_content_type_update_fields(): code duplication for the 3 fields. Can't you just walk through them in a loop?
5. user_content_type.module: I would suggest to move the page callback to a separate include file to keep the module file short.
6. _user_content_type_build_all(): this will not scale if a site has many users, the page request will just die with a PHP timeout. You should mention on the triggering button that it will only work for small sets of users. A better way is the batch API to work through larger sets.
7. _user_content_type_build_all(): why do you have to delete everything first? That is quite a waste and will break existing links to the user nodes.
8. Not sure if this should live in the existing usernode project or not - but I guess this is sufficiently different.

Not application critical application blockers, so ...

Thanks for your contribution, Martin Schauer!

I updated your account so you can promote this to a full project and also create new projects as either a sandbox or a "full" project.

Here are some recommended readings to help with excellent maintainership:

• Dries' post on Responsible maintainers
• Best practices for creating and maintaining projects
• Maintaining a drupal.org project with Git
• Commit messages - providing history and credit
• Release naming conventions.
• Helping maintainers in the issue queues

You can find lots more contributors chatting on IRC in #drupal-contribute. So, come hang out and stay involved!

Thanks, also, for your patience with the review process. Anyone is welcome to participate in the review process. Please consider reviewing other projects that are pending review. I encourage you to learn more about that process and join the group of reviewers.

Thanks to the dedicated reviewer(s) as well.