Support from Acquia helps fund testing for Drupal Acquia logo

Comments

jorap’s picture

Here is the corrected patch explaining the issue.

lambic’s picture

Status: Needs review » Reviewed & tested by the community

Looks good to me.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, aliased_multi_site_extra_port_comments.patch, failed testing.

jhr’s picture

Title: Adjust documentation for aliased multi sites to include a custom port example. » Adjust documentation for aliased multi sites to include more info and examples.
Component: base system » documentation
Status: Needs work » Needs review
FileSize
7.4 KB

Added some more info, and doxygen markup to make the variable blocks look better on api.d.o.

Status: Needs review » Needs work
Issue tags: -multisite

The last submitted patch, 1018324-adjust-aliased-multisite-documentation.patch, failed testing.

jhr’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 1018324-adjust-aliased-multisite-documentation.patch, failed testing.

jhr’s picture

Status: Needs work » Needs review
Issue tags: +multisite
jhodgdon’s picture

Status: Needs review » Needs work

Thanks for reviving this old issue! I think the latest patch needs a bit of work though:

a) This code violates our coding standards:

- *   'devexample.com' => 'example.com',
+ *   'devexample.com'    => 'example.com',

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)

+ * When index.php is called, Drupal tries to discover an appropriate
+ * configuration directory based on the website's hostname, port, and pathname.
+ * This file allows you to define a set of aliases that map hostnames, ports and

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)

+ * //'The url to alias'  => 'A directory within the sites directory',

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)

+ * the same as the domain of the live server. Since Drupal stores file paths
+ * into the database (files, system table, etc.) this will ensure the paths

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

jhr’s picture

Status: Needs work » Needs review
Issue tags: +Novice
FileSize
10.97 KB

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

joachim’s picture

Status: Needs review » Needs work

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

+++ b/core/includes/bootstrap.inc
@@ -547,17 +547,24 @@ function timer_stop($name) {
- *   'devexample.com' => 'example.com',
+ *   'devexample.com'    => 'example.com',
  *   'localhost.example' => 'example.com',
+ *   '8080.localhost'    => 'example.com',
+ *   '8080.example.com.mysite.test' => 'example.com',
  * );

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.

jhodgdon’s picture

Just because it exists doesn't mean it's according to our standards. See
http://drupal.org/coding-standards#array

joachim’s picture

It's done *lots*, and it's useful. I think the standards perhaps need adjusting.

jhodgdon’s picture

OK. Then we need another issue to discuss updating the standards. Please mark it component = documentation, tag = "coding standards". :)

joachim’s picture

jhr’s picture

Status: Needs work » Needs review

#10 changed status, and #11 changed the status back

jaredsmith’s picture

Status: Needs review » Reviewed & tested by the community

This looks good to me. Read through the changes in patch #10 (they make sense to me), and made sure the patch applies cleanly.

jhodgdon’s picture

Status: Reviewed & tested by the community » Needs work

Sorry, but this is not quite ready to commit:

a)

+++ b/sites/example.sites.php
...
+ * This file allows you to define a set of aliases that map hostnames, ports and

Needs comma before "and".

b)

+ * Aliases are defined in an associative array named $sites. To create a
+ * directory alias for http://www.drupal.org:8080/mysite/test whose
+ * configuration file is in sites/example.com the array should look like:

This needs a comma before "the" in the last line.

c)

+ * @code
+ * $sites = array(
+ * //'The URL to alias(formatted like example)' => 'A directory within the sites directory',
+ *   '8080.www.drupal.org.mysite.test' => 'example.com',
+ * );

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)

+ * The URL, http://www.drupal.org:8080/mysite/test/, could be a symbolic link or
+ * an Apache Alias directive that points to the Drupal root containing
+ * index.php. An alias could also be created for a subdomain. See the online
+ * Drupal installation guide for more information on setting up domains,
+ * subdomains, and subdirectories.
+ *
+ * @see default.settings.php
+ * @see conf_path()
+ * @see http://drupal.org/documentation/install/multi-site
  */

How about turning the text "online Drupal installation guide" into a link using @link?

e)

 * @code
+ * $sites = array(
+ *   'The URL to alias(formatted like examples)' => 'A directory within the sites directory'
+ * );
+ * @endcode

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)

+ */
\ No newline at end of file
lambic’s picture

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

jhodgdon’s picture

RE #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.

disasm’s picture

Status: Needs work » Needs review
FileSize
2.6 KB
10.97 KB

attached is a patch and interdiff

tstoeckler’s picture

Status: Needs review » Needs work

The patch contains some trailing whitespace.

tstoeckler’s picture

+++ b/sites/example.sites.php
@@ -4,40 +4,65 @@
+ * Aliases are defined in an associative array named $sites. The array is ¶
...
+ * http://www.drupal.org:8080/mysite/test whose configuration file is in ¶

There we go. Sorry, I intended to include this in the post above.

disasm’s picture

Status: Needs work » Needs review
FileSize
10.96 KB

attached fixes those two trailing whitespace issues.

tstoeckler’s picture

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

disasm’s picture

Assigned: Unassigned » disasm
jhodgdon’s picture

Status: Needs review » Needs work

We'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

disasm’s picture

Assigned: disasm » Unassigned
Status: Needs work » Needs review
FileSize
600 bytes
10.96 KB

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

jhodgdon’s picture

Status: Needs review » Needs work

That change is fine, thanks! Leaving at "needs work" for the other items in #27.

jhodgdon’s picture

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

+ * prior to scanning for directories. It should define an associative array
+ * named $sites, which maps domains to directories. $sites should be in the form
+ * of:
+ * @code
+ * $sites = array(
+ *   'The URL to alias' => 'A directory within the sites directory',
+ * );
+ * @endcode

Let's use the same wording that is used for this elsewhere in the patch (which is more accurate):

+ * Aliases are defined in an associative array named $sites. The array is
+ * written in the format: '<port>.<domain>.<path>' => 'directory'.
+ * As an example, to create a directory alias for:
+ * http://www.drupal.org:8080/mysite/test whose configuration file is in
+ * sites/example.com, the array should be written as such:
+ * @code
+ * $sites = array(
+ *   '8080.www.drupal.org.mysite.test' => 'example.com',
+ * );
+ * @endcode

b) in default.settings.php:

+ * configuration file found will be used and any  others will be ignored. If no

This line has two spaces between any and others.

c) still in default.settings.php:

+ * http://www.drupal.org:8080/mysite/test/, the 'settings.php' is searched in
+ * the following directories:
...
+ * Notice that if you are installing on a non-standard port number, prefix the

- is searched -> file is searched for
- Notice -> Note

d) In reading the documentation for find_conf_path():

/**
  * Finds the appropriate configuration directory for a given host and path.
  *
+ * Finds a matching configuration directory by stripping the website's hostname
+ * from left to right and pathname from right to left. The first configuration
+ * file found will be used and the remaining ones will be ignored. If no
+ * configuration file is found, returns a default value '$confdir/default'. See
+ * default.settings.php for examples on how the URL is converted to a directory.

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"):

* The configuration file to be loaded is based upon the rules below.
  *
+ * If the multisite aliasing file named sites/sites.php is present, it will be
+ * loaded, and the aliases in the array $sites will override the default
+ * directory rules below. See sites/example.sites.php for more information about
+ * aliases.

f) In example.sites.php:

+ * As an example, to create a directory alias for:
+ * http://www.drupal.org:8080/mysite/test whose configuration file is in
+ * sites/example.com, the array should be written as such:

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:

*/
 
 /**
- * Multi-site directory aliasing:
+ * Example multi-site directory aliases.

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


 /**
- * Multi-site directory aliasing:
+ * Example multi-site directory aliases.
+ *
+ * $sites should define an associative array which maps domains to directories.
+ * It should be in the form of:
+ * @code
+ * $sites = array(
+ *    '<port>.<domain>.<path>' => 'directory',
+ * );
+ * @endcode
+ *

That's it -- hopefully all of these requests are clear. :)

disasm’s picture

Status: Needs work » Needs review
FileSize
5.91 KB
10.86 KB

I think I got everything in #30. Let me know if anything else needs changed.

jhodgdon’s picture

Status: Needs review » Needs work

Looks pretty good, thanks! I would just change the following:

a) in find_conf_path() docs:

+ * If a file named sites.php is present in the $confdir, it will be loaded
+ * prior to scanning for directories.
+ *
+ * Aliases are defined in an associative array named $sites. The array is
+ * written in the format: '<port>.<domain>.<path>' => 'directory'.
+ * As an example, to create a directory alias for:
+ * http://www.drupal.org:8080/mysite/test whose configuration file is in
+ * sites/example.com, the array should be written as such:

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:

+ * The following examples look for a site configuration in sites/example.com
+ * @code
+ * URL: http://dev.drupal.org
+ * $sites['dev.drupal.org'] = 'example.com';
+ *
+ * URL: http://localhost/example
+ * $sites['localhost.example'] = 'example.com';
+ *
+ * URL: http://localhost:8080/example
+ * $sites['8080.localhost.example'] = 'example.com';
  *
+ * URL: http://www.drupal.org:8080/mysite/test/
+ * $sites['8080.www.drupal.org.mysite.test'] = 'example.com';
+ * @endcode

But if you think those are not useful, we can leave them out. ???

disasm’s picture

Status: Needs work » Needs review
FileSize
2.98 KB
11.52 KB

attached are the requested changes.

jhodgdon’s picture

Status: Needs review » Needs work

Looks excellent! The only thing now needed is a : at this line almost at the bottom of the patch (in example.sites.php):

+ * The following examples look for a site configuration in sites/example.com

Thanks!

webbykat’s picture

Status: Needs work » Needs review
FileSize
12.03 KB

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

jhodgdon’s picture

Status: Needs review » Needs work

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

-  // Export the following settings.php variables to the global namespace
+  // Export the following settings.php variables to the global namespace:

? 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!

Albert Volkman’s picture

Status: Needs work » Needs review
FileSize
1.26 KB
12.24 KB

Re-rolled.

jhodgdon’s picture

Status: Needs review » Reviewed & tested by the community

Looks 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. :)

webchick’s picture

Assigned: Unassigned » jhodgdon

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

jhodgdon’s picture

Version: 8.x-dev » 7.x-dev
Status: Reviewed & tested by the community » Patch (to be ported)

There 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!

jhodgdon’s picture

Assigned: jhodgdon » Unassigned

I'm not planning to do this port -- issue was assigned to me for committing -- hopefully someone will take it up! :)

Albert Volkman’s picture

Status: Patch (to be ported) » Needs review
FileSize
10.29 KB

D7 backport.

jhodgdon’s picture

Status: Needs review » Reviewed & tested by the community

Looks good! I'll get that patch committed shortly unless someone else beats me to it.

jhodgdon’s picture

Status: Reviewed & tested by the community » Fixed

Committed. Thanks all!

Status: Fixed » Closed (fixed)
Issue tags: -multisite, -Novice

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