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.

CommentFileSizeAuthor
#46 interdiff-2568439-35-46.txt437 bytesr_sharma08
#46 profile-readme-change-2568439-46.patch908 bytesr_sharma08
#36 profile-readme-change-2568439-35.patch911 byteshussainweb
#32 profile-readme-change-2568439-32.patch929 byteshussainweb
#32 interdiff-28-32.txt812 byteshussainweb
#29 interdiff-2568439-26-28.txt698 bytesdrupal.ninja03
#28 profile-readme-change-2568439-28.patch929 bytesdrupal.ninja03
#26 interdiff-2568439-23-26.txt696 bytesanil280988
#26 profile-readme-change-2568439-26.patch911 bytesanil280988
#25 interdiff-2568439-23-25.txt345 bytesanchal29
#25 profile-readme-change-2568439-25.patch887 bytesanchal29
#23 interdiff-2568439-17-23.txt683 bytesanchal29
#23 profile-readme-change-2568439-23.patch857 bytesanchal29
#20 profile-readme-change-2568439-20.patch564 bytesamit.drupal
#17 profile_readme-change-2568439-17.patch558 bytespriya.chat
#15 profile_readme-change-2568439-15.patch1.01 KBpriya.chat
#5 profile_readme-change-2568439-5-D8.patch565 bytessnehi
#2 incorrect_statement-2.patch516 bytesjoyceg
profiles_readme_incorrect_description.patch516 bytestedbow

Comments

tedbow created an issue. See original summary.

joyceg’s picture

joyceg commented
StatusFileSize
new incorrect_statement-2.patch516 bytes
joyceg’s picture

joyceg commented
Assigned: Unassigned » joyceg
Status: Active » Needs review
snehi’s picture

snehi commented
Assigned: joyceg » snehi
Status: Needs review » Needs work

I think only one line should be deleted that is last line.

They only impact the installation of your site.

snehi’s picture

snehi commented
Status: Needs work » Needs review
StatusFileSize
new profile_readme-change-2568439-5-D8.patch565 bytes
tedbow’s picture

tedbow
he/him
Primary language English
Location Ithaca, NY, USA
commented
Status: Needs review » Needs work 
+++ b/profiles/README.txt
@@ -6,8 +6,7 @@ WHAT TO PLACE IN THIS DIRECTORY?
+They only impact the installation of your site.

The 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:

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.

snehi’s picture

snehi commented

Any Suggestion on this want to close this minor one.

jhodgdon’s picture

jhodgdon
she/her
Primary language English
commented

We still need a patch that corrects the facts. #6 has a suggestion about what still needs to be fixed... and the issue summary...

snehi’s picture

snehi commented
Assigned: snehi » Unassigned
jhodgdon’s picture

jhodgdon
she/her
Primary language English
commented
Issue tags: +Novice
cilefen’s picture

cilefen commented
Title: profiles/README.txt incorrectly states that installation profiles don't effect 'already running' sites. » profiles/README.txt incorrectly states that installation profiles do not 'have any effect on' already running sites.
jhodgdon’s picture

jhodgdon
she/her
Primary language English
commented

Actually 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.

priya.chat’s picture

priya.chat commented
Assigned: Unassigned » priya.chat
r_sharma08’s picture

r_sharma08 commented

This 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.

priya.chat’s picture

priya.chat commented
Status: Needs work » Needs review
StatusFileSize
new profile_readme-change-2568439-15.patch1.01 KB

Hello ,
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.

jhodgdon’s picture

jhodgdon
she/her
Primary language English
commented
Status: Needs review » Needs work

Thanks, but:

+++ b/profiles/README.txt
@@ -6,8 +6,7 @@ WHAT TO PLACE IN THIS DIRECTORY?
+They impact the installation of your site. ¶

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.

priya.chat’s picture

priya.chat commented
Status: Needs work » Needs review
StatusFileSize
new profile_readme-change-2568439-17.patch558 bytes

Thanks @jhodgdon for your correction, my mistake. Now I have been corrected the patch and also remived the extra space. Please review this.

kaushalkishorejaiswal’s picture

kaushalkishorejaiswal commented
Assigned: priya.chat » kaushalkishorejaiswal
jhodgdon’s picture

jhodgdon
she/her
Primary language English
commented
Status: Needs review » Needs work

OK, good.

Now can we get the other problems identified in #2625074: Inconsistencies in the "profiles" directory README fixed too? Thanks!

amit.drupal’s picture

amit.drupal commented
StatusFileSize
new profile-readme-change-2568439-20.patch564 bytes
kaushalkishorejaiswal’s picture

kaushalkishorejaiswal commented
Assigned: kaushalkishorejaiswal » Unassigned
snehi’s picture

snehi 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

anchal29’s picture

anchal29 commented
Assigned: Unassigned » anchal29
Status: Needs work » Needs review
StatusFileSize
new profile-readme-change-2568439-23.patch857 bytes
new interdiff-2568439-17-23.txt683 bytes

I would like to work on the issue.
Here is a combined patch for this issue and issue #2625074.

snehi’s picture

snehi commented
Status: Needs review » Needs work 
  1. +++ b/profiles/README.txt
@@ -1,13 +1,12 @@
+installation is complete.

    installation of drupal is completed for the first time.

    Don't know is the correct? 

  2. +++ b/profiles/README.txt
@@ -1,13 +1,12 @@
+They impact the installation of your site.

    As well the running site i think.

anchal29’s picture

anchal29 commented
Status: Needs work » Needs review
StatusFileSize
new profile-readme-change-2568439-25.patch887 bytes
new interdiff-2568439-23-25.txt345 bytes

Here is a patch.
But I'm not sure what to change here.

+++ b/profiles/README.txt
@@ -1,13 +1,12 @@
+They impact the installation of your site.

As it affects the installation and could affect running sites also.

anil280988’s picture

anil280988 commented
StatusFileSize
new profile-readme-change-2568439-26.patch911 bytes
new interdiff-2568439-23-26.txt696 bytes

Hi 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.

snehi’s picture

snehi commented
Status: Needs review » Needs work 
+++ b/profiles/README.txt
@@ -1,13 +1,12 @@
+They impact the installation of your site as well as running site.

They word should not be used.
I think.

drupal.ninja03’s picture

drupal.ninja03 commented
Status: Needs work » Needs review
StatusFileSize
new profile-readme-change-2568439-28.patch929 bytes

Hi snehi,
Changed "They" to "Installation profiles".

drupal.ninja03’s picture

drupal.ninja03 commented
StatusFileSize
new interdiff-2568439-26-28.txt698 bytes

Adding interdiff.

snehi’s picture

snehi commented
Status: Needs review » Reviewed & tested by the community

Looks ok to me, making it RTBC now.
Anyone can change the status if i missed something.

jhodgdon’s picture

jhodgdon
she/her
Primary language English
commented
Status: Reviewed & tested by the community » Needs work

Thanks 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:

  1. +++ b/profiles/README.txt
@@ -1,13 +1,13 @@
 Installation profiles define additional steps that run after the base
-installation provided by Drupal core when Drupal is first installed.
+installation of Drupal is completed for the first time.

    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? 

  2. +++ b/profiles/README.txt
@@ -1,13 +1,13 @@
+distribution. Installation profiles impact the installation of your site as
+well as running site.

    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!

hussainweb’s picture

hussainweb
Primary language English
commented
Status: Needs work » Needs review
StatusFileSize
new interdiff-28-32.txt812 bytes
new profile-readme-change-2568439-32.patch929 bytes

How 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.

jhodgdon’s picture

jhodgdon
she/her
Primary language English
commented
Status: Needs review » Needs work

Thanks 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.

  1. +++ b/profiles/README.txt
@@ -1,13 +1,13 @@
+installation of Drupal is completed for the first time.

    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? 

  2. +++ b/profiles/README.txt
@@ -1,13 +1,13 @@
+distribution. Installation profiles impact the installation of your site as
+well as running site.

    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. ;)

snehi’s picture

snehi commented
Assigned: anchal29 » snehi
hussainweb’s picture

hussainweb
Primary language English
commented

I think I paired the interdiff with the wrong patch. Both those comments are addressed (as seen in the interdiff). Let me try again.

hussainweb’s picture

hussainweb
Primary language English
commented
Status: Needs work » Needs review
StatusFileSize
new profile-readme-change-2568439-35.patch911 bytes

Yes, 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.

Nnabueze’s picture

Nnabueze commented
Status: Needs review » Closed (fixed)

applied profile-readme-change-2568439-35.patch and content of README file is changed as needed

snehi’s picture

snehi commented
Assigned: snehi » Unassigned
Status: Closed (fixed) » Needs review

How to close this ?

cilefen’s picture

cilefen commented

@Nnabueze Issues are not considered fixed until the code change is committed to the repository.

snehi’s picture

snehi commented
Status: Needs review » Needs work 
+++ b/profiles/README.txt
@@ -1,13 +1,13 @@
+functionality and change the behavior of the website.

Some installation profiles also affect the site after the initial installation.
will be better.

hussainweb’s picture

hussainweb
Primary language English
commented

Some installation profiles also affect the site after the initial installation.
will be better.

I am assuming you mean that you want to replace the following sentence with "Some installation profiles also affect the site after the initial installation."

+++ b/profiles/README.txt
@@ -1,13 +1,13 @@
+installation of Drupal is completed. They may also offer additional
+functionality and change the behavior of the website.

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?

snehi’s picture

snehi commented

Yes you catched it right, but i may be wrong.
lets see what other thinks and

website

word should be replaced by

site

.
Don't you think so ?
Thanks for your passion for contribution.

Nnabueze’s picture

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?

hussainweb’s picture

hussainweb
Primary language English
commented

Thanks, 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.

hussainweb’s picture

hussainweb
Primary language English
commented

@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.

r_sharma08’s picture

r_sharma08 commented
Status: Needs work » Needs review
StatusFileSize
new profile-readme-change-2568439-46.patch908 bytes
new interdiff-2568439-35-46.txt437 bytes

Changes done into the attached patch.
Please review.

jhodgdon’s picture

jhodgdon
she/her
Primary language English
commented
Status: Needs review » Reviewed & tested by the community

Thanks! This last patch looks good to me. Very clear, and accurate, and concise.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Reviewed & tested by the community » Fixed

Committed 9de84a8 and pushed to 8.0.x and 8.1.x. Thanks!

  • alexpott committed d7f1630 on 8.1.x 
    Issue #2568439 by anchal29, hussainweb, priya.chat, r_sharma08, drupal....

  • alexpott committed 9de84a8 on 
    Issue #2568439 by anchal29, hussainweb, priya.chat, r_sharma08, drupal....

Status: Fixed » Closed (fixed)

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

amit.drupal’s picture

amit.drupal commented