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

[REF] Remove civicrm.files override for WordPress to fix issues with… #17868

Merged

Conversation

seamuslee001
Copy link
Contributor

… users struggling to find extensions

Overview

This removes the civicrm.files token override which seems to have caused some issues for users on upgrade to 5.27 see this thread https://chat.civicrm.org/civicrm/pl/mmqxdjnap3b4ubbeqwnn7kdxgc

Before

civicrm.files gives off potentially the wrong file path

After

civicrm.files gives off the correct civicrm files file path

ping @jusfreeman @kcristiano @christianwach

@civibot
Copy link

civibot bot commented Jul 16, 2020

(Standard links)

@civibot civibot bot added the 5.28 label Jul 16, 2020
@kcristiano
Copy link
Member

kcristiano commented Jul 16, 2020

@seamuslee001 it's hard to find the issue and steps to reproduce from a Mattermost thread.

I ran the initial PR through all my test sites (Single site, Multi-site, mapped domains, wp-content dir moved, wp in a sub directory) which is quite substantial.

I'd like to understand the problem we are solving and how to recreate before I dive in there again.

From what I read we are still trying to support a directory path we deprecated in version 4.6. It's far better to document moving to the proper paths or adding overrides for custom locations. And I consider wp-content/plugins/files/ a custom location

@totten
Copy link
Member

totten commented Jul 16, 2020

Yeah, this patch is a bit rough - feels like we're just as likely to break something else.

Whenever there's a bug in path/URL computation, I would suggest running this to help diagnose the problem: https://gist.github.com/totten/5997ff53088b159e7275f761e52f9516

The main thing is to get the report running in the problematic environment. (The suggestions in the first 5 or so lines for some specific environments - but that bit has to be adapted.)

@agileware-justin
Copy link
Contributor

Thanks @seamuslee001

We have seen this problem with WordPress sites on CiviCRM 5.27 where:
Settings - Resource URLs, when using the [civicrm.files] token was returning the file path, not the URL to the CiviCRM uploads directory. This obviously breaks a lot of things.

The Settings - Upload Directories were set correctly and in the most recent case, was using an absolute path for the directories.

Removing the [civicrm.files] token use in the Resource URLs was the solution in all cases.

@agileware-justin
Copy link
Contributor

For these WordPress sites, these are single site installs. Not multi-site, very vanilla WordPress set up and standard hosting - nothing special.

We've seen the problem and there's been recent reports of the problem in Mattermost to boot.

@kcristiano
Copy link
Member

@agileware-justin Any chance you can run Tim's script on a site that had issues.

This is output from a 5.27.2 local site of mine: https://gist.github.com/kcristiano/ff8098d3f3d0ea8f82c55fa362dc62d1
The site was built with buildkit, but it's the same as other sites.

This is handbuilt master https://gist.github.com/kcristiano/b056b2394233ffb3adf1d0d378838b04
With and without the PR are the same.

My concern about removing the code from CRM_Utils_System_WordPress is that we'll use the system default and not a WP specific function. This has plagued us for years (and obviously still is). I think we should be determining URLs using CMS functions wherever possible and override when needed.

@agileware-justin
Copy link
Contributor

agileware-justin commented Jul 17, 2020

@kcristiano yes we can clone the site and re-test. Will do that next week (or on the weekend), if that's OK.

@christianwach
Copy link
Member

@seamuslee001 I would not recommend removing this block of code. It overrides the default method for discovering CiviCRM's files directory (which calls the flawed CRM_Utils_System_WordPress::getDefaultFileStorage() method) to return correct paths from WordPress itself.

Admittedly, the paths are only correct for CiviCRM installs which do not use the legacy locations for the civicrm.settings.php file and the files directory (as is noted in the MM thread) but these haven't been the default locations since CiviCRM 4.7beta4 and should not be supported any more. It's fairly simple to relocate these to the current locations as @kcristiano explains in the thread.

Removing this code would mean that any installs which do use the [civicrm.files] token would fall back to that flawed method. It would be better to document what's required for updating legacy installs than to break up-to-date installs.

@eileenmcnaughton
Copy link
Contributor

@seamuslee001 I think it's fairly clear this is not going into the rc as is - move to gitlab?

@eileenmcnaughton
Copy link
Contributor

@agileware-justin can you make sure there is an adequate gitlab for this - this doesn't seem like 'the fix' but we definitely need to track fixing it - if we have a gitlab (with a regression label) we can close this without losing sight of it

@seamuslee001
Copy link
Contributor Author

@christianwach sure appreciate that feeling, I was mostly looking at it from the approach of 'stuff started failing according to people i know in 5.27 what changed?'

@eileenmcnaughton @agileware-justin @kcristiano @christianwach closing for now and have opened the gitlab here https://lab.civicrm.org/dev/wordpress/-/issues/66

@christianwach
Copy link
Member

@seamuslee001 👍

@agileware-justin
Copy link
Contributor

Posted a comment with the results of testing here, https://lab.civicrm.org/dev/wordpress/-/issues/66#note_40826

@kcristiano kcristiano reopened this Jul 28, 2020
@kcristiano
Copy link
Member

reopening based on https://lab.civicrm.org/dev/wordpress/-/issues/66

@kcristiano
Copy link
Member

jenkins re-test this please

@totten
Copy link
Member

totten commented Jul 29, 2020

jenkins test this please

@seamuslee001
Copy link
Contributor Author

Test failure is unrelated I take it this should be merged right @kcristiano ?

@kcristiano
Copy link
Member

@seamuslee001 I have done many r-run on my testing setups and agree it is good to merge.

@seamuslee001 seamuslee001 merged commit df6303e into civicrm:5.28 Jul 29, 2020
@seamuslee001 seamuslee001 deleted the remove_civicrm_files_override branch July 29, 2020 21:27
@seamuslee001
Copy link
Contributor Author

thanks @kcristiano

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants