The file profiles/README.txt states
Installation profiles are generally provided as part of a Drupal distribution.
They only impact the installation of your site. They do not have any effect on
an already running site.
The wording "already running site" site is a little weird but assuming this means "already installed" it is still incorrect.
Installation profiles very often will be used to update a site after installation through implementing hook_update_N
They can also affect a site after installation by implementing other hooks in the .profile file.
I have not yet written a Installation profile for Drupal 8 so I copied the standard profile and made it standard2 under /profile to make sure this is all still correct.
Sure enough in standard2.profile I can put:
/**
* Implements hook_form_alter().
*/
function standard2_form_alter(&$form, &$form_state, $form_id) {
drupal_set_message('this is form ' . $form_id );
}
This outputs the id of every form on the site.
Of course this is a silly example but if you check out the Drupal 7 Commerce Kickstart profile file you can see that implements a few hooks that will affect the site after install such as hook_features_api_alter, hook_field_default_field_bases_alter, hook_field_default_field_instances_alter.
It seems that the 2 lines quoted at the top of this issue should just deleted.
Comment | File | Size | Author |
---|---|---|---|
#46 | interdiff-2568439-35-46.txt | 437 bytes | r_sharma08 |
#46 | profile-readme-change-2568439-46.patch | 908 bytes | r_sharma08 |
#36 | profile-readme-change-2568439-35.patch | 911 bytes | hussainweb |
#32 | interdiff-28-32.txt | 812 bytes | hussainweb |
Comments
Comment #2
joyceg CreditAttribution: joyceg commentedComment #3
joyceg CreditAttribution: joyceg commentedComment #4
snehi CreditAttribution: snehi at Publicis Sapient for Publicis Sapient commentedI think only one line should be deleted that is last line.
They only impact the installation of your site.
Comment #5
snehi CreditAttribution: snehi at Publicis Sapient for Publicis Sapient commentedComment #6
tedbowThe point of this issue is that this line is factually incorrect. As stated in the description of the this issue there a multiple examples where they actually do affect things other than installation.
As noted:
Comment #7
snehi CreditAttribution: snehi at Publicis Sapient for Publicis Sapient commentedAny Suggestion on this want to close this minor one.
Comment #8
jhodgdonWe still need a patch that corrects the facts. #6 has a suggestion about what still needs to be fixed... and the issue summary...
Comment #9
snehi CreditAttribution: snehi at Publicis Sapient for Publicis Sapient commentedComment #10
jhodgdonComment #11
cilefen CreditAttribution: cilefen commentedComment #12
jhodgdonActually this issue needs to be combined with #2625074: Inconsistencies in the "profiles" directory README. Mark one as a duplicate and combine the patches. Make sure everyone gets credit.
Comment #13
priya.chat CreditAttribution: priya.chat at Publicis Sapient for Publicis Sapient commentedComment #14
r_sharma08 CreditAttribution: r_sharma08 at Publicis Sapient for Publicis Sapient commentedThis issue has been combined with issue #2625074.
We should remove that part "They only impact the installation of your site and have no effect on a site that is already installed." of description entirely from README in profile directory.
Comment #15
priya.chat CreditAttribution: priya.chat at Publicis Sapient for Publicis Sapient commentedHello ,
I have added the patch as I have removed the last line and also change one more line as "They impact the installation of your site."
Please review my patch.
Comment #16
jhodgdonThanks, but:
This line has an extra space at the end.
Also the patch has something unrelated to this issue in it, and not all of the stuff identified in the other issue that is now combined has been addressed.
Comment #17
priya.chat CreditAttribution: priya.chat at Publicis Sapient for Publicis Sapient commentedThanks @jhodgdon for your correction, my mistake. Now I have been corrected the patch and also remived the extra space. Please review this.
Comment #18
kaushalkishorejaiswal CreditAttribution: kaushalkishorejaiswal as a volunteer and at Publicis Sapient for Publicis Sapient commentedComment #19
jhodgdonOK, good.
Now can we get the other problems identified in #2625074: Inconsistencies in the "profiles" directory README fixed too? Thanks!
Comment #20
amit.drupal CreditAttribution: amit.drupal at gai Technologies Pvt Ltd commentedComment #21
kaushalkishorejaiswal CreditAttribution: kaushalkishorejaiswal as a volunteer and at Publicis Sapient for Publicis Sapient commentedComment #22
snehi CreditAttribution: snehi at Publicis Sapient for Publicis Sapient commented@Amit.drupal please provide the interdiff. It will help us to review the current patch.
On the another hand please try to work on assigned issue, address all the things which are listed in #19
Comment #23
anchal29 CreditAttribution: anchal29 as a volunteer commentedI would like to work on the issue.
Here is a combined patch for this issue and issue #2625074.
Comment #24
snehi CreditAttribution: snehi as a volunteer and at Publicis Sapient for Publicis Sapient commentedinstallation of drupal is completed for the first time.
Don't know is the correct?
As well the running site i think.
Comment #25
anchal29 CreditAttribution: anchal29 as a volunteer commentedHere is a patch.
But I'm not sure what to change here.
As it affects the installation and could affect running sites also.
Comment #26
anil280988 CreditAttribution: anil280988 at Publicis Sapient for Publicis Sapient commentedHi anchal29,
For #24.2, as it impact the running site (means already installed one) so it should be "They impact the installation of your site as well as running site."
Feel free to change anything if it doesn't feel right.
Comment #27
snehi CreditAttribution: snehi as a volunteer and at Publicis Sapient for Publicis Sapient commentedThey word should not be used.
I think.
Comment #28
drupal.ninja03 CreditAttribution: drupal.ninja03 at Publicis Sapient for Publicis Sapient commentedHi snehi,
Changed "They" to "Installation profiles".
Comment #29
drupal.ninja03 CreditAttribution: drupal.ninja03 at Publicis Sapient for Publicis Sapient commentedAdding interdiff.
Comment #30
snehi CreditAttribution: snehi as a volunteer and at Publicis Sapient for Publicis Sapient commentedLooks ok to me, making it RTBC now.
Anyone can change the status if i missed something.
Comment #31
jhodgdonThanks for the patches and reviews!
So let's see. Back to the issue summary for this as well as the linked issue that was closed as a duplicate. What we were supposed to cover:
a) Take out the part about "do not impact running site", which is incorrect.
b) Rewrite the first sentence so it is less run-on and clearer.
c) Make the sentence about installation profiles being parts of distributions clearer.
So looking at the patch... I would say that (a) could be improved, but (b) and (c) are OK. Specific thoughts:
So... This is definitely the *primary* purpose of installation profiles, but as noted in the issue summary, they can do more than just this, so I'm not sure we're really satisfying the goals of this issue with this introduction.
Can we fix this up better please?
This last sentence is redundant to the first paragraph and I think we decided above that we should just remove it? It's also not at all related to the rest of this paragraph, or to the header of "what to place in this directory".
Thanks!
Comment #32
hussainwebHow does this look? I removed ' for the first time' from the first line entirely as it did not make much sense. I also removed the last line from the second paragraph as pointed out in #31.
Comment #33
jhodgdonThanks for the new patch! However, I do not think that the goals of this issue are still being satisfied. Maybe you did not read my previous review comment? It seems that it was ignored...
Let me try again.
Hm. So what does "for the first time" mean here? You only install Drupal in a given Drupal site once.
I think we can take out "for the first time" and not lose anything in the meaning. Right?
Can we *please* just completely remove the sentence starting "Installation profiles impact...". I think it just leads to confusion.
Plus, this section is about "What to place in this directory", not about "What is an installation profile". Right?
So if we really want to make this point about already running sites, let's add it to the first paragraph of this README.
To do that, I think we can add a sentence to the end ***of the first paragraph at the top of the README*** (NOT in the "What to place in this directory" section)... something like this:
Some installation profiles also affect the site after the initial installation.
Note: I think "affect" is a better choice of words for this than "impact" -- a bit less violent sounding. ;)
Comment #34
snehi CreditAttribution: snehi as a volunteer and at Publicis Sapient for Publicis Sapient commentedComment #35
hussainwebI think I paired the interdiff with the wrong patch. Both those comments are addressed (as seen in the interdiff). Let me try again.
Comment #36
hussainwebYes, it was what I thought. The patch in #32 is identical to the one in #28. Sorry for the mistake. @jhodgdon, I didn't ignore the comment, at least, not intentionally.
Here is the proper patch. The interdiff in #32 is still good.
Comment #37
Nnabueze CreditAttribution: Nnabueze commentedapplied profile-readme-change-2568439-35.patch and content of README file is changed as needed
Comment #38
snehi CreditAttribution: snehi as a volunteer and at Publicis Sapient for Publicis Sapient commentedHow to close this ?
Comment #39
cilefen CreditAttribution: cilefen commented@Nnabueze Issues are not considered fixed until the code change is committed to the repository.
Comment #40
snehi CreditAttribution: snehi as a volunteer and at Publicis Sapient for Publicis Sapient commentedSome installation profiles also affect the site after the initial installation.
will be better.
Comment #41
hussainwebI am assuming you mean that you want to replace the following sentence with "Some installation profiles also affect the site after the initial installation."
I think that the one in patch is more accurate and specific. Also, when you say "after the initial installation", it kind of implies that it only affects _after_ the installation, not during it. Can you elaborate a bit more on what you want it to say?
Comment #42
snehi CreditAttribution: snehi as a volunteer and at Publicis Sapient for Publicis Sapient commentedYes you catched it right, but i may be wrong.
lets see what other thinks and
word should be replaced by
.
Don't you think so ?
Thanks for your passion for contribution.
Comment #43
Nnabueze CreditAttribution: Nnabueze commented@cilefen, @snehi Thanks for the correction. I am new here. Is there a guideline page I could read on to know how things are done?
Comment #44
hussainwebThanks, I don't care too much between 'site' and 'website'. Both get the meaning across. I think we can try to keep it consistent but as long as it gets the meaning across clearly, I am fine.
Comment #45
hussainweb@Nnabueze: You can read more about the entire patch process here - https://www.drupal.org/patch. Don't forget to read the sub-pages as well.
Comment #46
r_sharma08 CreditAttribution: r_sharma08 at Publicis Sapient for Publicis Sapient commentedChanges done into the attached patch.
Please review.
Comment #47
jhodgdonThanks! This last patch looks good to me. Very clear, and accurate, and concise.
Comment #48
alexpottCommitted 9de84a8 and pushed to 8.0.x and 8.1.x. Thanks!
Comment #52
amit.drupal CreditAttribution: amit.drupal as a volunteer and at gai Technologies Pvt Ltd for gai Technologies Pvt Ltd commented