Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Also search for pg4wp data files in the WordPress root directory #13

Merged
merged 1 commit into from
Mar 17, 2018
Merged

Conversation

ntninja
Copy link
Contributor

@ntninja ntninja commented Mar 16, 2018

Title says it all, see the first commit. Also search for the pg4wp data files in the WordPress root directory in order to better support installations where these are bundled along with WordPress.

@kevinoid
Copy link
Collaborator

Thanks for the PR!

Could you explain why pg4wp would be bundled in the root directory rather than the wp-content directory and why it would be desirable to support this?

The comment in your second commit suggests that '' = NULL returns a different value in MySQL than PostgreSQL, which hasn't been my experience (both result in "unknown"). Could you explain the issue you are trying to fix, along with the query which behaves differently?

There are some queries which would be mis-translated by your current approach (e.g. col = '''quoted''') and ones which would be missed (e.g. COALESCE(col1, col2) = ''). Neither of those is a deal-breaker since the code already has many such issues. But it's worth considering if there is a different way to fix the issue you are having with fewer risks.

@ntninja
Copy link
Contributor Author

ntninja commented Mar 16, 2018

Could you explain why pg4wp would be bundled in the root directory rather than the wp-content directory and why it would be desirable to support this?

Basically for this: https://hub.docker.com/r/ntninja/wordpress-postgresql/
(Yes, I can of course easily work around you not merging it, but I'd like to keep all components as pristine as possible.)

The comment in your second commit suggests that '' = NULL returns a different value in MySQL than PostgreSQL, which hasn't been my experience (both result in "unknown"). Could you explain the issue you are trying to fix, along with the query which behaves differently?

I have no idea what you are refering to with '' = NULL, but I definitely see different results for

SELECT 'true' WHERE '' = '';  -- Returns 'true'

vs

SELECT 'true' WHERE '' IS NULL;  -- Returns nothing

in my PostgreSQL database browser. I don't have any MySQL server here to test, but the issue I tried to solve was definitely related to this. I'll extract a sample query for you.

There are some queries which would be mis-translated by your current approach (e.g. col = '''quoted''')

I didn't know MySQL has multiline string syntax, will fix this!

and ones which would be missed (e.g. COALESCE(col1, col2) = '').

Since these won't be corrupted either I don't see the issue here.

@ntninja
Copy link
Contributor Author

ntninja commented Mar 16, 2018

Some script that is part of page theme of a page I'm maintaining queries the database for a list of all posts whose images should be displayed as part of a slider animation. qTranslate-X then hooks into all queries and ensures that only posts with a localized or no description will be retrieved:

SELECT wp_posts.id
FROM   wp_posts
WHERE  1 = 1
       AND wp_posts.post_type = 'slide'
       AND (( wp_posts.post_status = 'publish' ))
       AND ( wp_posts.post_content = ''
              OR wp_posts.post_content LIKE '%![:de!]%' escape '!'
              OR wp_posts.post_content LIKE '%%'
              OR ( wp_posts.post_content NOT LIKE '%![:!]%' escape '!'
                   AND wp_posts.post_content NOT LIKE '%%' ) )
ORDER  BY wp_posts.post_date DESC
LIMIT  0, 5 

All of the relevant database entries have no description in this case however:

ID post_author post_date post_date_gmt post_content post_title post_excerpt post_status
185 1 "2013-08-14 07:00:58" "2013-08-14 05:00:58" [null] […omitted…] [null] "publish"
178 1 "2013-08-14 08:00:26" "2013-08-14 06:00:26" [null] […omitted…] [null] "publish"
177 1 "2013-08-14 09:00:49" "2013-08-14 07:00:49" [null] […omitted…] [null] "publish"

Since in PostgreSQL NULL != '' these entries will not match and the slider will be empty. I hope that clearifies the problem.

@kevinoid
Copy link
Collaborator

Could you explain why pg4wp would be bundled in the root directory rather than the wp-content directory and why it would be desirable to support this?

Basically for this: https://hub.docker.com/r/ntninja/wordpress-postgresql/
(Yes, I can of course easily work around you not merging it, but I'd like to keep all components as pristine as possible.)

Cool! Thanks for putting that together, it looks useful!

I still don't understand the advantage of putting pg4wp in the Wordpress root directory. Couldn't you just change ADD ./postgresql-for-wordpress/pg4wp /usr/src/wordpress/pg4wp to ADD ./postgresql-for-wordpress/pg4wp /usr/src/wordpress/wp-content/pg4wp/? What's the advantage of having it in the Wordpress root?

There are some queries which would be mis-translated by your current approach (e.g. col = '''quoted''')

I didn't know MySQL has multiline string syntax, will fix this!

I'm afraid I have mislead you here. It's not multi-line syntax but escaping of apostrophe. For example 'I didn''t explain clearly' So '''hi''' is the string hi in single quotes.

and ones which would be missed (e.g. COALESCE(col1, col2) = '').

Since these won't be corrupted either I don't see the issue here.

If both col1 and col2 are NULL then COALESCE(col1, col2) = '' would have the same result as col1 = ''. Perhaps a better example would be (col1) = '' where the parentheses have no effect but would cause the regexp not to match. (col1) could be replaced by any arbitrary expression which returns NULL. I don't think it's solvable with a regexp and would require a proper parser, so I wouldn't spend much effort trying to fix it. Just something to be aware of

Thanks for the example data and query, that's very helpful! From your description I created the following script:

DROP TABLE IF EXISTS wp_posts;
CREATE TABLE wp_posts (
	id INT PRIMARY KEY,
	post_date TIMESTAMP NOT NULL,
	post_content TEXT,
	post_status VARCHAR(20) NOT NULL,
	post_type VARCHAR(20) NOT NULL
);
INSERT INTO wp_posts (id, post_date, post_content, post_status, post_type)
VALUES (185, '2013-08-14 07:00:58', NULL, 'publish', 'slide'),
	(178, '2013-08-14 08:00:26', NULL, 'publish', 'slide'),
	(177, '2013-08-14 09:00:49', NULL, 'publish', 'slide');
SELECT wp_posts.id
FROM   wp_posts
WHERE  1 = 1
       AND wp_posts.post_type = 'slide'
       AND (( wp_posts.post_status = 'publish' ))
       AND ( wp_posts.post_content = ''
              OR wp_posts.post_content LIKE '%![:de!]%' escape '!'
              OR wp_posts.post_content LIKE '%%'
              OR ( wp_posts.post_content NOT LIKE '%![:!]%' escape '!'
                   AND wp_posts.post_content NOT LIKE '%%' ) )
ORDER  BY wp_posts.post_date DESC;

This script produced the same result for me on MySQL and PostgreSQL (no rows). Changing wp_posts.post_content = '' to wp_posts.post_content IS NULL also the same result on MySQL and PostgreSQL (3 rows). Have I misunderstood your description, or does this produce a different result on your server?

@ntninja
Copy link
Contributor Author

ntninja commented Mar 17, 2018

Cool! Thanks for putting that together, it looks useful!

Glad you see it this way. Other people respond quite differently at times…

I still don't understand the advantage of putting pg4wp in the Wordpress root directory. Couldn't you just change ADD ./postgresql-for-wordpress/pg4wp /usr/src/wordpress/pg4wp to ADD ./postgresql-for-wordpress/pg4wp /usr/src/wordpress/wp-content/pg4wp/? What's the advantage of having it in the Wordpress root?

Mostly because the wp-config and wp-content subdirectories are reserved for user modifications, so I want to dump as few files as possible into those directories that are not intended for user configuration or the result of actions taken by the users. db.php qualifies as user configuration, which is why it will be copied exactly once into wp-content and then is left unchanged after that (and it's not like we have any choice on where to put it anyways). pg4wp on the other hand would have to be overwritten each time the image is started to ensure that its files are kept up-to-date – if users were to make changes to this they'd be in for an unplesant surprise that we'd like to avoid. Also I want to fully support the use-case where use a custom version of PostgreSQL for WordPress in their data directory for debugging and non-upstreamable modifications without.

This script produced the same result for me on MySQL and PostgreSQL (no rows). Changing wp_posts.post_content = '' to wp_posts.post_content IS NULL also the same result on MySQL and PostgreSQL (3 rows). Have I misunderstood your description, or does this produce a different result on your server?

I don't have any MySQL server to test, but I just tried creating a new, empty post in WordPress and wp_posts.post_content was indeed '' and not NULL. I guess that means those NULL entries either the result of a (possibly now fixed) bug in WordPress or introduced due to some wrong database configuration or some issue during the last save/restore. Sorry to have bothered you about this I'll fix this on my end and drop that commit…

@ntninja ntninja changed the title Fix issues with qTranslateX (and possibly other plugins using ident='') Also search for pg4wp data files in the WordPress root directory Mar 17, 2018
@kevinoid
Copy link
Collaborator

Mostly because the wp-config and wp-content subdirectories are reserved for user modifications

Thanks for explaining! That makes sense and sounds like a good plan and a good enough reason to support pg4wp in the Wordpress root directory going forward to me. Will merge.

I don't have any MySQL server to test, but I just tried creating a new, empty post in WordPress and wp_posts.post_content was indeed '' and not NULL. I guess that means those NULL entries either the result of a (possibly now fixed) bug in WordPress or introduced due to some wrong database configuration or some issue during the last save/restore. Sorry to have bothered you about this I'll fix this on my end and drop that commit…

No worries. That happens. If the NULL values reappear or you are able to figure out how the NULL comparison behavior differed, feel free to open another issue or PR and I'll help isolate and address it.

Thanks again for the PR and for taking the time to work through all of my questions in detail. Much appreciated!

@kevinoid kevinoid merged commit 3fa4370 into PostgreSQL-For-Wordpress:wordpress4-compat Mar 17, 2018
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.

2 participants