Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 PST on 31 March 2024, to get $100 off your ticket.
As requested by redndahead, I suggest to make a slight documentation change in the aliased multi site instructions by including a custom port example. Please refer to my patch at: http://drupal.org/node/231298#comment-3910010.
Comments
Comment #1
jorap CreditAttribution: jorap commentedHere is the corrected patch explaining the issue.
Comment #2
lambic CreditAttribution: lambic commentedLooks good to me.
Comment #4
jhr CreditAttribution: jhr commentedAdded some more info, and doxygen markup to make the variable blocks look better on api.d.o.
Comment #6
jhr CreditAttribution: jhr commented#4: 1018324-adjust-aliased-multisite-documentation.patch queued for re-testing.
Comment #8
jhr CreditAttribution: jhr commented#4: 1018324-adjust-aliased-multisite-documentation.patch queued for re-testing.
Comment #9
jhodgdonThanks for reviving this old issue! I think the latest patch needs a bit of work though:
a) This code violates our coding standards:
I understand that you want things to line up, but since we don't do that in Drupal, we shouldn't do that in our examples either.
b)
We don't "call" index.php. Someone visits a page on the site. And Drupal doesn't "try" to discover an appropriate config file, it chooses one. Then in the next line, it says "this file", but there is no file referred to (the previous sentence refers to a directory). So this paragraph needs a rewrite.
c)
Should be a space after the // for readability, and "url" should be "URL". And see comment (a) here too. Hmmm... Actually the array key is not actually the URL to alias, because one example is "localhost.example", and this is for localhost/example. And none of these are URLs anyway -- URLs are things like http://whatever.com. So you need to find a better way to describe these if you are going to put a comment into the array.
d)
---> stores file paths in the database [need to fix this in two places in the patch]
e) Do not repeat documentation. There are 3 spots in this patch with the same documentation. It would be much better to use a line like "See abc() for details" rather than having to maintain the same documentation in 3 places.
Comment #10
jhr CreditAttribution: jhr commenteda) ok
b) ok
c) Original intent was to improve the readers ability to determine which was the URL and which was the directory. Since there really isn't a name for this type of formatting, I just added '(formatted like examples)' to show that it's not a normal URL.
d)k
e)I trimmed down the core functions and made those comments more appropriately explain what the function does and point to the default.settings.php, and examples.sites.php. There's still some repetition of (c), but that's to provide a quick reference down towards the $sites declaration similar to how settings.default.php has a $databases example, then another example near the $databases declaration.
Comment #11
joachim CreditAttribution: joachim commentedRegarding the lining up, I can find tons of examples of doing this to arrays in Drupal core; "ack ' {2,}=>'" runs to pages and pages of results.
The lining up looks good to me and helps readability -- though if we are lining up then the last line needs to be included in that too.
20 days to next Drupal core point release.
Comment #12
jhodgdonJust because it exists doesn't mean it's according to our standards. See
http://drupal.org/coding-standards#array
Comment #13
joachim CreditAttribution: joachim commentedIt's done *lots*, and it's useful. I think the standards perhaps need adjusting.
Comment #14
jhodgdonOK. Then we need another issue to discuss updating the standards. Please mark it component = documentation, tag = "coding standards". :)
Comment #15
joachim CreditAttribution: joachim commentedDone: #1475154: coding standards: lining up large arrays with padding
Comment #16
jhr CreditAttribution: jhr commented#10 changed status, and #11 changed the status back
Comment #17
jaredsmith CreditAttribution: jaredsmith commentedThis looks good to me. Read through the changes in patch #10 (they make sense to me), and made sure the patch applies cleanly.
Comment #18
jhodgdonSorry, but this is not quite ready to commit:
a)
Needs comma before "and".
b)
This needs a comma before "the" in the last line.
c)
That in-code comment makes no sense at all to me, and actually I don't think we should put in-code comments inside @code tags at all. Put the explanation in the text instead.
d)
How about turning the text "online Drupal installation guide" into a link using @link?
e)
Needs a space before (formatted like examples).... but what does that actually mean -- what examples? It's not really a URL at all (especially if it has a port)... I think this just needs to be reworded somehow (not sure how).
f)
Comment #19
lambic CreditAttribution: lambic commenteda) launches you straight into an Oxford Comma debate, which neither side ever wins, so best just to pick one and leave it alone.
The other suggestions all make sense though.
Comment #20
jhodgdonRE #19 - the Drupal project long ago picked a standard on the Oxford Comma Debate:
http://drupal.org/style-guide/content#english
Our documentation style standard is to require the comma.
Comment #21
disasm CreditAttribution: disasm commentedattached is a patch and interdiff
Comment #22
tstoecklerThe patch contains some trailing whitespace.
Comment #23
tstoecklerThere we go. Sorry, I intended to include this in the post above.
Comment #24
disasm CreditAttribution: disasm commentedattached fixes those two trailing whitespace issues.
Comment #25
tstoecklerThis looks very good.
I only read this patch with the "Hide deletions" options, and only briefly looked at the actual changes, so I am not marking this RTBC.
Comment #26
disasm CreditAttribution: disasm commentedComment #27
jhodgdonWe're pretty close! #18-a wasn't done, and I think the whole thing could use a little copy editing. There are several places that are still a bit awkward in how they are worded. I also think some of this would benefit from using our standard documentation list syntax.
http://drupal.org/node/1354#lists
Comment #28
disasm CreditAttribution: disasm commentedattached is a patch fixing the issue in 18a that also existed in bootstrap.inc. Rewording documentation isn't my strongest suit, so I'm going to unassign this patch so someone else can take a look at it.
Comment #29
jhodgdonThat change is fine, thanks! Leaving at "needs work" for the other items in #27.
Comment #30
jhodgdonAs requested in IRC, I've just looked through the patch in #28 to see what still needs to be fixed. Here's a list:
a) find_conf_path() docs -- section on the sites.php file:
Let's use the same wording that is used for this elsewhere in the patch (which is more accurate):
b) in default.settings.php:
This line has two spaces between any and others.
c) still in default.settings.php:
- is searched -> file is searched for
- Notice -> Note
d) In reading the documentation for find_conf_path():
I just looked at the code for this function, and this is actually not quite accurate. There is an undocumented parameter $require_settings (this needs an @param to be added to the documentation). If that parameter is TRUE (default), then a directory has to have a settings.php file in it to be considered to be a match. If it is FALSE, then the directory just has to exist to be considered a match. This paragraph should explain this (or reference the $require_settings parameter documentation). This paragraph just refers to "configuration file" being found, which is not accurate (and also doesn't explicitly say it is looking for a file named settings.php).
e) I think we should join these two paragraphs in default.settings.php (start the second sentence with "However"):
f) In example.sites.php:
Rewrite this as:
As an example, to map http://www.drupal.org:8080/mysite/test to the configuration directory sites/example.con, the $sites array should be defined as:
g) In example.sites.php:
This second documentation block will not be recognized by the API module (or displayed on api.drupal.org), because it is not documenting a function, global variable, etc. It just needs to be made part of the previous documentation block instead. (When you do this, move the @see lines down to the bottom of the new, larger documentation block.) Actually... I think that all of this can be deleted completely (the information is presented above):
That's it -- hopefully all of these requests are clear. :)
Comment #31
disasm CreditAttribution: disasm commentedI think I got everything in #30. Let me know if anything else needs changed.
Comment #32
jhodgdonLooks pretty good, thanks! I would just change the following:
a) in find_conf_path() docs:
I guess I wasn't quite clear, sorry! I think these two paragraphs should be combined... Something like:
If a file named sites.php is present in the $confdir, it will be loaded prior to scanning for directories. That file can define aliases in an associative array named $sites. The array is written [...]
b) Also, this same section of documentation needs the correction from (f) in #30.
c) (c) from #30 needs to be redone. "Notice" became "Not" instead of "Note", and the "file is searched for" replacement for "is searched" wasn't put in.
d) in (g) from #30 I didn't really mean to completely eliminate the second documentation block with the examples... I think we should keep these lines from the previous patch:
But if you think those are not useful, we can leave them out. ???
Comment #33
disasm CreditAttribution: disasm commentedattached are the requested changes.
Comment #34
jhodgdonLooks excellent! The only thing now needed is a : at this line almost at the bottom of the patch (in example.sites.php):
Thanks!
Comment #35
webbykat CreditAttribution: webbykat commentedI made the change requested in comment #34. I also noticed that there was another similar problem in bootstrap.inc so I updated that as well ("// Export the following settings.php variables to the global namespace" now has a colon at the end). Here you go.
Comment #36
jhodgdonUh oh, I just noticed that in the first hunk of this patch, the @see is misplaced (we put all @see lines at the end of a docblock).
Also, did this patch make this new change:
? If so, please change that to a ".". In-code comments are supposed to be sentences. Actually, either just don't change this line, or make it into a sentence.
Otherwise, this patch looks good!
Comment #37
Albert Volkman CreditAttribution: Albert Volkman commentedRe-rolled.
Comment #38
jhodgdonLooks good now, thanks! This might have to wait for a few days to be committed, as there's a large "avoid commit conflicts" issue waiting... also I am currently on a slow internet connection, and traveling for the next week, so if one of the other core committers wants to do it that might be good. :)
Comment #39
webchickSince Jennifer's already been involved in this issue, and since there are A LOT of docs here, I'm going to assign this to her to drive home. But just so you know, this doesn't conflict in any way with any of the "Avoid commit conflicts" patches atm.
Comment #40
jhodgdonThere don't seem to be any lingering "avoid commit conflicts" issues for D8 at the moment, and I've committed the above patch there. The patch doesn't apply to d7, so we need a backport. Thanks!
Comment #41
jhodgdonI'm not planning to do this port -- issue was assigned to me for committing -- hopefully someone will take it up! :)
Comment #42
Albert Volkman CreditAttribution: Albert Volkman commentedD7 backport.
Comment #43
jhodgdonLooks good! I'll get that patch committed shortly unless someone else beats me to it.
Comment #44
jhodgdonCommitted. Thanks all!