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.
Comment | File | Size | Author |
---|---|---|---|
#38 | company-1127418-38.patch | 16.1 KB | tim.plunkett |
#7 | company-place.jpg | 18.55 KB | chrislynch42 |
#7 | Company1.png | 36.23 KB | chrislynch42 |
#7 | Company2.png | 41.26 KB | chrislynch42 |
Comments
Comment #1
chrislynch42 CreditAttribution: chrislynch42 commentedHere is the link to the company module.
Comment #2
sreynen CreditAttribution: sreynen commentedComment #3
rteijeiro CreditAttribution: rteijeiro commentedHi.
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.
Comment #4
gregglesI 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.
Comment #5
rteijeiro CreditAttribution: rteijeiro commentedOk, 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.
Comment #6
chrislynch42 CreditAttribution: chrislynch42 commentedI have added variable_del to the uninstall. What advantage do you see in using drupal_write_record over using db_query?
Comment #7
chrislynch42 CreditAttribution: chrislynch42 commentedI 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.
Comment #8
sreynen CreditAttribution: sreynen commentedchrislynch42, 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.
Comment #9
chrislynch42 CreditAttribution: chrislynch42 commentedChange hook_insert to using drupal_write_record.
Comment #10
rteijeiro CreditAttribution: rteijeiro commentedOk, 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.
Comment #11
chrislynch42 CreditAttribution: chrislynch42 commentedReadme and License files added.
Comment #12
gregglesThe LICENSE.txt file should actually be removed. It is added automatically by the packager.
Comment #13
chrislynch42 CreditAttribution: chrislynch42 commentedLicense file removed:)
Comment #14
rteijeiro CreditAttribution: rteijeiro commentedSorry, but I think you mistake the README.txt file ;)
This file is from the Path redirect module.
Kind regards.
Comment #15
chrislynch42 CreditAttribution: chrislynch42 commentedI used the the path Readme as the basis for mine. I made the necessary changes.
Comment #16
13rac1 CreditAttribution: 13rac1 commentedvariable_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.'db_query("UPDATE {system} SET weight=1000 WHERE name='company'");
Why is the weight being adjusted for this 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?
Comment #17
13rac1 CreditAttribution: 13rac1 commentedComment #18
chrislynch42 CreditAttribution: chrislynch42 commented1) 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.
Comment #19
chrislynch42 CreditAttribution: chrislynch42 commentedForgot to change the status.
Comment #20
gregglesThis 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.
Comment #21
chrislynch42 CreditAttribution: chrislynch42 commented@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:)
Comment #22
chrislynch42 CreditAttribution: chrislynch42 commentedSession variable seems to work just fine. Changes made.
Comment #23
gregglesI'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.
Comment #24
13rac1 CreditAttribution: 13rac1 commentedComment #25
chrislynch42 CreditAttribution: chrislynch42 commented1) 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.
Comment #26
chrislynch42 CreditAttribution: chrislynch42 commentedComment #27
sreynen CreditAttribution: sreynen commentedSorry, 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.
Comment #28
chrislynch42 CreditAttribution: chrislynch42 commentedPing
Comment #29
juampynr CreditAttribution: juampynr commentedI 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?
Comment #30
13rac1 CreditAttribution: 13rac1 commented@juampy: I agree.
Comment #31
chrislynch42 CreditAttribution: chrislynch42 commentedI disagree but will not spend any more of my time trying to convince you otherwise.
Comment #32
sreynen CreditAttribution: sreynen commentedchrislynch42, 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.
Comment #33
juampynr CreditAttribution: juampynr commentedI 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.
Comment #34
sreynen CreditAttribution: sreynen commentedjuampy, 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.
Comment #35
juampynr CreditAttribution: juampynr commentedOK, will take it into account ;-D.
What is the #IRC group name for project applications?
Comment #36
sreynen CreditAttribution: sreynen commented#drupal-codereview -- it's on the group page.
Comment #37
jthorson CreditAttribution: jthorson commentedMarking RTBC based on #23, a really quick scan, and verification that the issues in #24 have been addressed.
Comment #38
tim.plunkettPlease 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.
Comment #39
MiSc CreditAttribution: MiSc commentedMarked 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)?
Comment #40
MiSc CreditAttribution: MiSc commentedThe application has been closed. If you would like to reopen it, you are free to do so.
See http://drupal.org/node/894256