I've gotten quite a bit over the last several years from Drupal and want to give a little back. Contributing to an open source project is one of my life goals. This module would be in the "Etities" package along with the Person module. I had considered breaking it down so that both the company and person modules could be based on an "entity" module but decided that much abstraction was counter productive.

This module is required by the Milan Module which is still in the sandbox. The idea is for the module to provide a basic module to support a variety of other modules. The Milan module uses the company module to track book publishers. A baseball card module could use the Company module as the basis to track baseball card manufacturers.

How would another module differentiate between publishers and baseball card manufacturers? First it would be differentiated by Taxonomy. Milan companys have the Seafarer->Book->Publisher taxonomy and that is how one knows a company is a publisher. Secondly Milan uses the hook_nodeapi to identify publishers and display them differently.

My searches did not show any similar modules. I am interested in any suggestions on how to make this module more useful to others.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

chrislynch42’s picture

Here is the link to the company module.

sreynen’s picture

Status: Active » Needs review
rteijeiro’s picture

Status: Needs review » Needs work

Hi.

I was looking your contributed modules and I think it will be better if you develop a complete solution than different modules.

I am not sure your module will be useful without the other modules.

Kind regards.

greggles’s picture

Status: Needs work » Needs review

I don't think it's a requirement of the review process to provide a complete solution.

This module seems to me to have enough purpose on its own.

It might be good to post some screenshots and explain how they add value to a site.

rteijeiro’s picture

Status: Needs review » Needs work

Ok, thank you for your annotation, Greg.

Just started to review the module and found some questions:

I think you should use the drupal_write_record instead the SQL statement in the company_insert function.

If you use drupal variables you shouldn't forget to delete them with the variable_del function in the hook_uninstall.

Kind regards.

chrislynch42’s picture

I have added variable_del to the uninstall. What advantage do you see in using drupal_write_record over using db_query?

chrislynch42’s picture

FileSize
41.26 KB
36.23 KB
18.55 KB

I have attached some screenshots as requested. I have attached a diagram which I think will be more helpful. Currently the company module provides the ability to add companies along with a description and to then list those companies. It is designed to easily allow changes in the future and to be of use to other modules.

This is the base functionality that three other modules currently need from the Company module. Milan is the only module of the three that I have placed in the sandbox to date. My plan is for Company to get accepted. Then Milan (The book tracking application). Then the DVD Tracker followed by the software key tracker.

Another issue is that the Company module needs to track addresses in order to be useful to a wider audience. It is healthier for Drupal as a whole if redundant code is kept to a minimum. So logically then it would be better to use an existing module to integrate with for addresses. But which address module should we choose? Addresses only has a Drupal 6 release and Address Field only has a Drupal 7 release.

I believe these issues would be easier to address once this module goes through this process. It would complicate any negotiations to change full module projects for one that is in sandbox.

sreynen’s picture

chrislynch42, drupal_write_record is a wrapper around db_insert and essentially writes the query for you based on the schema definition. This has the advantage of a level of abstraction away from the database, which is generally a good thing because it allows for dealing with minor differences in different database backends. Using higher-level database functions is a good habit to get into, though I wouldn't expect it to make any real difference in this specific case.

chrislynch42’s picture

Status: Needs work » Needs review

Change hook_insert to using drupal_write_record.

rteijeiro’s picture

Status: Needs review » Needs work

Ok, nice work!!

You should only add a README.txt file with the project installation, features and requirements. After that I think it will be ready to become a full project.

Kind regards.

chrislynch42’s picture

Status: Needs work » Needs review

Readme and License files added.

greggles’s picture

Status: Needs review » Needs work

The LICENSE.txt file should actually be removed. It is added automatically by the packager.

chrislynch42’s picture

Status: Needs work » Needs review

License file removed:)

rteijeiro’s picture

Status: Needs review » Needs work

Sorry, but I think you mistake the README.txt file ;)

This file is from the Path redirect module.

Kind regards.

chrislynch42’s picture

Status: Needs work » Needs review

I used the the path Readme as the basis for mine. I made the necessary changes.

13rac1’s picture

  1. // $Id$ is used throughout the files. It should be removed.
  2. company.form.inc - 'create pd_recluse entries' permission, it is not created by this module
  3. company.module:26 - Use this node to record companys., "companies" is misspelled.
  4. company.module:20 - * @see hook_perm() should be * Implements hook_node_perm(). same issue everywhere else this type of comment is used.
  5. company.pages.inc:19 - variable_set('company_breadcrumb' , array(l(t('Home') , NULL) , l(t('Company') , 'company'))); It seems this variable is set every time the company list is used to make the breadcrumbs? There must be a better way to do that.
  6. company.install:31 - 'Holds book companys for the milan project.' incorrect db description.
  7. company.info - Why is this module in the package Entities?
  8. company.install:15 - 'db_query("UPDATE {system} SET weight=1000 WHERE name='company'"); Why is the weight being adjusted for this module?
  9. Please run this through the coder.module.

I'm not sure I understand why this module exists. It seems as though all the functionality could be created with a content type, CCK, and Views. Can you explain what this does that isn't provided by a combination of core and those modules?

13rac1’s picture

Status: Needs review » Needs work
chrislynch42’s picture

1) Removed all.
2) Removed all.
3) Corrected all.
4) Replaced all @see with Implements. Did not change hook_perm or others to hook_node_****.
5) This module is designed to be easily subsumed by other modules. Using the variable allows for other modules to integrate the Company module functionality into itself seamlessly. At least that's the goal:)
6) Corrected.
7) A person and a company are entities. They should be grouped together. I can see automated systems eventually also being grouped as an entity as well. I have already addressed why person and company do not have a parent module.
8) The weight needs to be set to something to help control when the module is executed in relation to all other modules. I plan to group all entity weights 1000.
9) Already done. I performed the task again. No significant changes.

I believe I have already addressed this above and in the project description. I have a need for it to finish creating other modules.

Any number of Developers and Power users could use views, cck and content type to create useful functionality. Why would the rest want to? That's work! I would prefer to purchase a home rather than build it myself.

chrislynch42’s picture

Status: Needs work » Needs review

Forgot to change the status.

greggles’s picture

company.pages.inc:19 - variable_set('company_breadcrumb' , array(l(t('Home') , NULL) , l(t('Company') , 'company'))); It seems this variable is set every time the company list is used to make the breadcrumbs? There must be a better way to do that.

This is a somewhat important point. @chrislynch42 - did you mean this code to set the variable or get the variable?

Doing a variable set clears the variable cache which can become a performance issue.

From your response I think your goal is actually to do a variable_get rather than a variable_set.

I've now reviewed company.pages.inc and it mostly looks good to me. I think with the most recent round of fixes this is nearly RTBC.

chrislynch42’s picture

@greggles,
I do mean to use "variable_set". If this causes performance issues then I will need to find an alternative solution. I suppose I could use session variables or perhaps cookies. I will play around with it and evaluate my options. The need in the application is to persist variables between modules for each user. Now that I say it out loud session variables sound pretty good:)

chrislynch42’s picture

Session variable seems to work just fine. Changes made.

greggles’s picture

Status: Needs review » Reviewed & tested by the community

I'm marking this RTBC and would like it to be in this status for a day or two before granting the role to allow others to review.

From my reviews of a few of the modules/files this looks good to me.

13rac1’s picture

Status: Reviewed & tested by the community » Needs work
  • DB table is titled: 'csl_company'. Should be just 'company' to match the module. What is csl?
  • Commented line in company.module: // node_form_submit($form , $form_state);
  • Company.module - company_form_submit() references hook_form_submit, but that hook doesn't exist.
  • Company.module - Lines 48-54 are spaced too much
  • global $user; is not used in company.pages.inc
  • company.module - Line 20 - Companies is still misspelled
  • company.module - Line 28 - company_perm() is implemented, but the permissions are never used.
chrislynch42’s picture

Status: Needs work » Needs review

1) Changed. Drupal needs a better naming standard. The database is table soup. package_name-module name or something similar. I didn't find this requirement in the coding standards. Went along with it because I haven't thought of something better.

2) Removed

3) Changed reference

4) Corrected.

5) Removed.

6) Corrected.

7) Permissions updated.

chrislynch42’s picture

Component: new project application » module
Status: Needs review » Reviewed & tested by the community
sreynen’s picture

Status: Reviewed & tested by the community » Needs review

Sorry, you can't mark your own application as reviewed. Someone else needs to do that. One of the people who have already looked at it should be able to look at the updates pretty quickly. If not, I'll do it.

chrislynch42’s picture

Ping

juampynr’s picture

I see this module more as a business solution to a very particular scenario rather than a tool or a general solution which could be easily reused by other people. The description of the module page does not convince me at all that this module could help me to solve a problem, but seems more that you are using it to track code changes of your custom modules.

Any more opinions about this?

13rac1’s picture

@juampy: I agree.

chrislynch42’s picture

Status: Needs review » Closed (won't fix)

I disagree but will not spend any more of my time trying to convince you otherwise.

sreynen’s picture

Status: Closed (won't fix) » Needs review

chrislynch42, to be clear, no one rejected your application. If you're no longer interested in the process, that's certainly your choice, but it sounds like you maybe misinterpreted comments as rejections. As greggles already said, nothing about this review process requires the project to be a general solution, so that doesn't really need any convincing.

juampynr’s picture

I am sorry if I sounded rude. I just want to help Drupal.org to be the best CMS, and I am worried that it could get over populated of modules if we do not debate new applications.

sreynen’s picture

juampy, that's fine. Just keep in mind that this is often the first experience with the wider contributor community for applicants, and debates aren't always the best introduction. There's a whole group and IRC channel for discussion about this process.

juampynr’s picture

OK, will take it into account ;-D.

What is the #IRC group name for project applications?

sreynen’s picture

#drupal-codereview -- it's on the group page.

jthorson’s picture

Status: Needs review » Reviewed & tested by the community

Marking RTBC based on #23, a really quick scan, and verification that the issues in #24 have been addressed.

tim.plunkett’s picture

Status: Reviewed & tested by the community » Needs work
FileSize
16.1 KB

Please set this back to RTBC after applying the following coding standards patch, and I will give you access and mark as fixed.
This is partially to help you out with the changes, and partially to see if you are still interested in maintaining the module.

MiSc’s picture

Marked with need work for 22 weeks. Tried to find contact adres for the applicant, but could not find any. On his personal page he says that he had dropped Drupal, mark as closed (won't fix)?

MiSc’s picture

Status: Needs work » Closed (won't fix)

The application has been closed. If you would like to reopen it, you are free to do so.
See http://drupal.org/node/894256