Skip to content

Conversation

@Purii
Copy link
Contributor

@Purii Purii commented Mar 8, 2015

Ensure the compatibility by adding the original comments. See http://codex.wordpress.org/htaccess

@md5
Copy link
Contributor

md5 commented Mar 9, 2015

It would have been nice if you had described the compatibility problems you faced or quoted the part of the page that discusses the "BEGIN WordPress" and "END WordPress" comments (since I couldn't actually find anything on that page that does). As it was, I ended up looking around the source code to figure out what the issue might be.

It looks like the relevant code is save_mod_rewrite_rules and insert_with_markers. In my quick search of the code, it appears to be involved in turning on and off the permalinks feature and in updating the rewrite rules when a you install WordPress upgrade.

@Purii
Copy link
Contributor Author

Purii commented Mar 9, 2015

I haven't looked which functions use this comments, because it is the default behaviour of WordPress. I thought it is enough to reference to the WordPress codex.
Sure, it doesn't break anything. It is just to make sure, that it looks exactly like a manual WP-Installation, to ensure that it works with future versions. And exactly.. I became aware of this behaviour, after changing the permalinks option. :)

@md5
Copy link
Contributor

md5 commented Mar 9, 2015

I haven't looked which functions use this comments, because it is the default behaviour of WordPress. I thought it is enough to reference to the WordPress codex.

That's fine, but that page in the WordPress codex doesn't say anything about those comments being required. It may imply that by including the comments in all the examples, but as far as I could tell WordPress doesn't actually document the BEGIN WordPress/END WordPress thing anywhere.

@yosifkit
Copy link
Member

yosifkit commented Mar 9, 2015

LGTM

@Purii
Copy link
Contributor Author

Purii commented Mar 10, 2015

After changing permalinks setting, the content is set twice. Sure, this doesn't matter since it's the same content. But it doesn't look nice and future-compatible. And yes, WP use this comments only in this functions. I'm sorry, but I don't understand the need to discuss the motives of WP to use this comments. (btw, the oldest occurrence I could find is in version 2.3, and there's no reason WHY - misc.php. Let's say it's common law? 😉)

RewriteEngine On
RewriteBase /
RewriteRule ^index\.php$ - [L]
RewriteCond %{REQUEST_FILENAME} !-f
RewriteCond %{REQUEST_FILENAME} !-d
RewriteRule . /index.php [L]

# BEGIN WordPress
<IfModule mod_rewrite.c>
RewriteEngine On
RewriteBase /
RewriteRule ^index\.php$ - [L]
RewriteCond %{REQUEST_FILENAME} !-f
RewriteCond %{REQUEST_FILENAME} !-d
RewriteRule . /index.php [L]
</IfModule>

# END WordPress

@tianon
Copy link
Member

tianon commented Mar 10, 2015

Seems legit to me, especially given that the WordPress source reads these markers. 👍

@tianon
Copy link
Member

tianon commented Mar 10, 2015

(despite it not being properly documented, it is all over the codex and the forums as an accepted "thing" WordPress does)

tianon added a commit that referenced this pull request Mar 10, 2015
Add original comments to .htaccess
@tianon tianon merged commit 7714595 into docker-library:master Mar 10, 2015
tianon added a commit to infosiftr/stackbrew that referenced this pull request Mar 20, 2015
- `buildpack-deps`: `squeeze` (docker-library/buildpack-deps#21), `liblzma-dev` (docker-library/buildpack-deps#19)
- `django`: 1.7.7
- `elasticsearch`: `java:7-jre` (docker-library/elasticsearch#4)
- `java`: 8u40 (GA of OpenJDK 8!)
- `logstash`: `logstash` user (docker-library/logstash#7)
- `mariadb`: remove 10.1 (MariaDB/mariadb-docker#7)
- `memcached`: LICENSE
- `mongo`: 3.0.1
- `percona`: 5.5.42, 5.6.23
- `postgres`: correct perms on `/run/postgresql` (docker-library/postgres#52)
- `rabbitmq`: 3.5.0
- `rails`: 4.2.1
- `sentry`: 7.4.1
- `wordpress`: add comments in `.htaccess` (docker-library/wordpress#65)
tianon added a commit to infosiftr/stackbrew that referenced this pull request Mar 20, 2015
- `buildpack-deps`: `squeeze` (docker-library/buildpack-deps#21), `liblzma-dev` (docker-library/buildpack-deps#19)
- `django`: 1.7.7
- `elasticsearch`: `java:7-jre` (docker-library/elasticsearch#4)
- `java`: 8u40 (GA of OpenJDK 8!)
- `logstash`: `logstash` user (docker-library/logstash#7)
- `mariadb`: remove 10.1 (MariaDB/mariadb-docker#7)
- `memcached`: LICENSE
- `mongo`: 3.0.1
- `percona`: 5.5.42, 5.6.23
- `postgres`: correct perms on `/run/postgresql` (docker-library/postgres#52)
- `rabbitmq`: 3.5.0
- `rails`: 4.2.1
- `sentry`: 7.4.1
- `wordpress`: add comments in `.htaccess` (docker-library/wordpress#65)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants