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

Wordpress improvements #96910

Closed
wants to merge 6 commits into from
Closed

Conversation

mohe2015
Copy link
Contributor

@mohe2015 mohe2015 commented Sep 1, 2020

Motivation for this change

Improve the wordpress service:

  • Add separate package that removes built-in themes and plugins as these can't be updated independently and also can't be removed otherwise.
  • Package auto-update script
  • Separate databases to allow multiple wordpress installations on one host by default
  • Add option for mutable wp-content

(edit: updated)

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS linux)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Ensured that relevant documentation is up to date
  • Fits CONTRIBUTING.md.

@ajs124
Copy link
Member

ajs124 commented Sep 2, 2020

I haven't gotten a chance to look at this, but wordpress can actually be served mostly read-only, from the nix store. One component that comes in handy for that is this abominationbeautiful piece of technology. It gives you each and every wordpress theme and plugin as a nix derivation.

@mohe2015 mohe2015 mentioned this pull request Oct 1, 2020
10 tasks
@mohe2015
Copy link
Contributor Author

mohe2015 commented Oct 2, 2020

@ajs124 Would it be possible for you to review my PR? I would really appreciate it.
Also regarding wp4nix I'm only using a few themes and plugins so I think my approach may also be fine (I think yours downloads all themes and plugins doesn't it?). I have to work on the translations in the future though.

@ajs124
Copy link
Member

ajs124 commented Oct 2, 2020

Sure, I'll try and give some useful feedback.
But first, wp4nix works by crawling the WordPress plugin directory API and SVN. It outputs a bunch of JSON files, in total roughly 25MB. These JSON files contain data like plugin/theme/... name, version, svn path and svn revision. They get parsed by the wp4nix default.nix, which exposes a bunch of attrsets, so you can use things like wordpressPackages.plugins.woocommerce.

The reason why we're using SVN is that with the setup they have there you can republish versions and people do. So the zip you download today does not necessarily have the same hash as the one you download tomorrow.
As a user of it, you'd download all the metadata (so 25MB JSON), but you only build and install the plugins and themes you actually use.

As for this PR:

  1. I can't really comment on the Apache2 config changes, because I don't remember when I last configured that software. Although it looks like they're just configuring a virtualhost as opposed to… whatever it's doing now? Just using the default host? What?
  2. The update scripts should probably moved up a folder, because you only need one for all themes and one for all plugins, right? You'd need to pass the name, but you can probably just do that with writeScript or something like that. That and they're "flawed" for the reasons outlined above, but you might get away with this approach here, since you would always substitute from the binary cache, once it's in there, anways.
  3. The database changes seem sensible enough, but they definitely need some kind of migration instructions. I know WordPress doesn't like if you change that kind of thing, so this needs to be tested well.
  4. You might run into issues with setting WP_HOME and WP_SITEURL the way you are doing here, if you ever change them. The reason for this is because WordPress (themes and plugins, idk if core as well) manage to take this and put it into the database. Which is why there are plugins like this.
  5. Does the nixos/tests test still pass without any changes?

That said, this still looks like an overall improvement 👍

@mohe2015
Copy link
Contributor Author

mohe2015 commented Oct 6, 2020

@ajs124 Thanks a lot for the review.

  1. I'm assuming you are referring to this commit: 39ed207?w=1. As far as I understand nix it only changes the service name from the services.wordpress.$name to the current host name the wordpress instance is reachable under, prepended with "wordpress-". This prevents service name clashes if you want to move another service to the old host name.
    Edit: There are some issues with naming: hostName usually actually means siteName (so not changeable) but I didn't want to further complicate this PR with variable name changes.
  2. I decided to remove the plugins and themes completely in favor of your approach.
  3. I added instructions to the theme path change.
  4. I think wordpress core stores the old url in the database but these variables always override the value. But you are right that some/most plugins and themes are incompatible. I removed setting WP_HOME and WP_SITEURL but still added instructions on how to change the hostName as these seem useful.
  5. yes

I added an option to allow a mutable wp-content directory but I don't know if this is wanted. I usually use this for trying out new themes and plugins.

I also added a mention to wp4nix if that is fine with you.

I also added a basic phpmyadmin implementation. Maybe I should put it into another pull request?

Honestly I assume that almost nobody seriously uses this service unmodified because theme updating is currently impossible without loosing settings.

P.S. It would be great if you could add a license to https://git.helsinki.tools/helsinki-systems/wp4nix. Also hosting on github may be beneficial so contributions are possible (If you want to deal with them).
I got a panic when running wp4nix with removed *.json and *.log files for wordpress 5.5: https://gist.github.com/mohe2015/27fe1bf6db92f3ad6a3a72fba8f596c8 If you want to you can look into it.

I would appreciate it if you could do a second round of reviewing.

Also I don't know how the stateVersion thing should be handled as I don't want to break unstable users' systems but it seems like this is the current way to go. I hope they are watching the repository closely. (I would need to change the stateVersion changes to 20.09 if this actually manages to get merged in the current release cycle, right?)

@mohe2015 mohe2015 marked this pull request as ready for review October 6, 2020 17:05
@ajs124
Copy link
Member

ajs124 commented Oct 13, 2020

As far as I understand nix it only changes the service name from the services.wordpress.$name to the current host name the wordpress instance is reachable under, prepended with "wordpress-". This prevents service name clashes if you want to move another service to the old host name.

Goes to show how much I know about the apache nixos module.

I added an option to allow a mutable wp-content directory but I don't know if this is wanted. I usually use this for trying out new themes and plugins.

AFAIR mutable content is often needed, because that's also where uploaded images and other files are stored.

I also added a basic phpmyadmin implementation. Maybe I should put it into another pull request?

Yeah, that should probably go in a separate PR.

Honestly I assume that almost nobody seriously uses this service unmodified because theme updating is currently impossible without loosing settings.

I kind of doubt anybody uses this, as well.

wp4nix is now licensed under the EUPL-1.2, but given that all code is either by me or @SlothOfAnarchy, we can also re-license it if anyone would prefer to have it under any other license.
Moving it to github is possible but not trivial, as we use gitlab ci and it's integrated into our deployment.

I got a panic when running wp4nix with removed *.json and *.log files for wordpress 5.5: https://gist.github.com/mohe2015/27fe1bf6db92f3ad6a3a72fba8f596c8 If you want to you can look into it.

If you want to regenerate the json files from scratch, you probably need to create empty (aka containing {}) json files, as it barely does any error handling.
Seeing as this was quite literally my first go project, I'm surprised it didn't set your computer on fire ^^
As far as I remember, it also doesn't actually do any version checking with the wordpress version you give it, because… well, because it wasn't necessary for the plugins and themes we use, probably. Maybe it gets used for the languages? We pull those from subversion as well, though.

Also I don't know how the stateVersion thing should be handled as I don't want to break unstable users' systems but it seems like this is the current way to go. I hope they are watching the repository closely. (I would need to change the stateVersion changes to 20.09 if this actually manages to get merged in the current release cycle, right?)

I haven't used stateVersion so far, maybe @aanderse has some insight on that.

@aanderse
Copy link
Member

You really would think no one uses this module... but I was shocked to learn that a number of people used the httpd submodules options before I deprecated/removed them. It goes to show you really don't know 🤷‍♂️

I only wrote this module to get rid of the httpd submodule options (hint: these options were bad) and don't have any personal interest in maintaining/using this module. I know nginx is much more popular compared to httpd (especially in NixOS) so maybe it makes sense to do a hard break and rewrite the module for nginx the way you would like it, if you're willing to maintain this module in NixOS/nixpkgs... what do you think?

@mohe2015
Copy link
Contributor Author

mohe2015 commented Oct 13, 2020

@aanderse The changes itself would be backwards compatible if the thing with the stateVersion is discussed.

I'm sorry but I won't rewrite this module. Also the better way would be to get a webserver-agnostic interface but I know it's gonna take a while until that's going to happen.

I would still like to get this merged though. The only issue left is what do do with the stateVersion because I don't want to break the system of unstable users. How should I handle that?

@mohe2015 mohe2015 force-pushed the wordpress-improvements branch 5 times, most recently from fe3ef48 to 8a3ce02 Compare October 13, 2020 21:00
@mohe2015 mohe2015 mentioned this pull request Feb 2, 2021
10 tasks
@mohe2015
Copy link
Contributor Author

mohe2015 commented Aug 3, 2021

I have rebased and dropped some commits that are not necessary in my opinion. I haven't tested again though yet.

The thing that really is necessary is 319e9b2. But the current migration is not a good way to manage this as this could prevent people from upgrading if they don't want to make these changes. I would propose adding some option like services.wordpress.usePluginAndThemeDirectoriesWithoutVersion = true; or so and documenting migration instructions there. Then maybe also adding a note to the release notes that this is recommended. What do you think?

@mohe2015
Copy link
Contributor Author

mohe2015 commented Aug 3, 2021

@GrahamcOfBorg test wordpress

@mohe2015
Copy link
Contributor Author

@GrahamcOfBorg test wordpress

@mohe2015
Copy link
Contributor Author

mohe2015 commented Aug 16, 2021

@onny Would love to get a review from you.

I removed the controversial change for now. I will try this in a separate PR.

Other reviews are also greatly appreciated.

@mohe2015 mohe2015 requested a review from aanderse August 16, 2021 22:40
@@ -483,7 +483,7 @@ in
''
( echo "CREATE USER IF NOT EXISTS '${user.name}'@'localhost' IDENTIFIED WITH ${if isMariaDB then "unix_socket" else "auth_socket"};"
${concatStringsSep "\n" (mapAttrsToList (database: permission: ''
echo "GRANT ${permission} ON ${database} TO '${user.name}'@'localhost';"
echo 'GRANT ${permission} ON ${database} TO `${user.name}`@`localhost`;'
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I checked (code-wise) that this shouldn't break anything but somebody else should probably also check that. Also I would run some other modules that use this before merging.

@@ -168,7 +191,7 @@ let

name = mkOption {
type = types.str;
default = "wordpress";
default = if lib.versionAtLeast config.system.stateVersion "21.11" then "wordpress-${name}" else "wordpress";
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe I should change this to

Suggested change
default = if lib.versionAtLeast config.system.stateVersion "21.11" then "wordpress-${name}" else "wordpress";
default = if lib.versionAtLeast config.system.stateVersion "21.11" then "wordpress-${replaceStrings [ "." ] [ "_" ] name}" else "wordpress";

@mohe2015
Copy link
Contributor Author

Superseded by #135966. Thank your to everyone who participated.

@mohe2015 mohe2015 closed this Oct 13, 2021
@mohe2015 mohe2015 deleted the wordpress-improvements branch November 28, 2021 20:26
@nixos-discourse
Copy link

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/prs-in-distress/3604/45

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

Successfully merging this pull request may close these issues.

None yet

6 participants