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/env: Set owner of wp-content to www-data #20406

Merged
merged 1 commit into from
Feb 28, 2020

Conversation

noisysocks
Copy link
Member

Fixes bug I noticed in #20403 (comment).

Makes the WordPress content directories (wp-content, wp-content/plugins, wp-content/themes) owned by the www-data user. This ensures that WordPress can write to these directories.

This is necessary when running wp-env with "core": null because Docker will automatically create these directories as the root user when binding volumes during docker-compose up, and docker-compose up doesn't support a --user option.

See docker-library/wordpress#436.

To test:

  1. Set "core": null in your .wp-env.json or .wp-env.override.json
  2. Run packages/env/bin/wp-env start
  3. Check that you can upload images, install themes, and install plugins

Makes the WordPress content directories (wp-content, wp-content/plugins,
wp-content/themes) owned by the www-data user. This ensures that WordPress
can write to these directories.

This is necessary when running wp-env with `"core": null` because Docker
will automatically create these directories as the root user when binding
volumes during `docker-compose up`, and `docker-compose up` doesn't support
the `-u` option.

See docker-library/wordpress#436.
@noisysocks noisysocks added [Type] Bug An existing feature does not function as intended [Tool] Env /packages/env labels Feb 24, 2020
@github-actions
Copy link

Size Change: 0 B

Total Size: 864 kB

ℹ️ View Unchanged
Filename Size Change
build/a11y/index.js 1.01 kB 0 B
build/annotations/index.js 3.43 kB 0 B
build/api-fetch/index.js 3.39 kB 0 B
build/autop/index.js 2.58 kB 0 B
build/blob/index.js 620 B 0 B
build/block-directory/index.js 6.02 kB 0 B
build/block-directory/style-rtl.css 760 B 0 B
build/block-directory/style.css 760 B 0 B
build/block-editor/index.js 104 kB 0 B
build/block-editor/style-rtl.css 9.78 kB 0 B
build/block-editor/style.css 9.77 kB 0 B
build/block-library/editor-rtl.css 7.67 kB 0 B
build/block-library/editor.css 7.67 kB 0 B
build/block-library/index.js 114 kB 0 B
build/block-library/style-rtl.css 7.47 kB 0 B
build/block-library/style.css 7.48 kB 0 B
build/block-library/theme-rtl.css 669 B 0 B
build/block-library/theme.css 671 B 0 B
build/block-serialization-default-parser/index.js 1.65 kB 0 B
build/block-serialization-spec-parser/index.js 3.1 kB 0 B
build/blocks/index.js 57.6 kB 0 B
build/components/index.js 190 kB 0 B
build/components/style-rtl.css 16.1 kB 0 B
build/components/style.css 16 kB 0 B
build/compose/index.js 5.76 kB 0 B
build/core-data/index.js 10.5 kB 0 B
build/data-controls/index.js 1.04 kB 0 B
build/data/index.js 8.22 kB 0 B
build/date/index.js 5.36 kB 0 B
build/deprecated/index.js 771 B 0 B
build/dom-ready/index.js 569 B 0 B
build/dom/index.js 3.06 kB 0 B
build/edit-post/index.js 90.7 kB 0 B
build/edit-post/style-rtl.css 8.7 kB 0 B
build/edit-post/style.css 8.69 kB 0 B
build/edit-site/index.js 4.58 kB 0 B
build/edit-site/style-rtl.css 2.77 kB 0 B
build/edit-site/style.css 2.76 kB 0 B
build/edit-widgets/index.js 4.36 kB 0 B
build/edit-widgets/style-rtl.css 2.8 kB 0 B
build/edit-widgets/style.css 2.79 kB 0 B
build/editor/editor-styles-rtl.css 327 B 0 B
build/editor/editor-styles.css 328 B 0 B
build/editor/index.js 45.1 kB 0 B
build/editor/style-rtl.css 4.13 kB 0 B
build/editor/style.css 4.11 kB 0 B
build/element/index.js 4.45 kB 0 B
build/escape-html/index.js 733 B 0 B
build/format-library/index.js 7.6 kB 0 B
build/format-library/style-rtl.css 500 B 0 B
build/format-library/style.css 501 B 0 B
build/hooks/index.js 1.92 kB 0 B
build/html-entities/index.js 621 B 0 B
build/i18n/index.js 3.45 kB 0 B
build/is-shallow-equal/index.js 711 B 0 B
build/keyboard-shortcuts/index.js 2.3 kB 0 B
build/keycodes/index.js 1.68 kB 0 B
build/list-reusable-blocks/index.js 2.99 kB 0 B
build/list-reusable-blocks/style-rtl.css 215 B 0 B
build/list-reusable-blocks/style.css 216 B 0 B
build/media-utils/index.js 4.85 kB 0 B
build/notices/index.js 1.57 kB 0 B
build/nux/index.js 3.02 kB 0 B
build/nux/style-rtl.css 616 B 0 B
build/nux/style.css 613 B 0 B
build/plugins/index.js 2.54 kB 0 B
build/primitives/index.js 1.49 kB 0 B
build/priority-queue/index.js 878 B 0 B
build/redux-routine/index.js 2.84 kB 0 B
build/rich-text/index.js 14.3 kB 0 B
build/server-side-render/index.js 2.54 kB 0 B
build/shortcode/index.js 1.7 kB 0 B
build/token-list/index.js 1.27 kB 0 B
build/url/index.js 4 kB 0 B
build/viewport/index.js 1.61 kB 0 B
build/warning/index.js 1.14 kB 0 B
build/wordcount/index.js 1.18 kB 0 B

compressed-size-action

@talldan
Copy link
Contributor

talldan commented Feb 24, 2020

This might also need to include the uploads folder - #20280 (comment)

I've cherry-picked that commit to my branch, so will see if the existing change solves that.

@noisysocks
Copy link
Member Author

noisysocks commented Feb 24, 2020

If #20280 is using the default .wp-env.json that's in Gutenberg then I don't think this fix will do anything. This PR fixes a bug that I've only seen occur when "core": null, and Gutenberg is by default "core": "WordPress/WordPress".

I did think about including wp-content/uploads here but I don't think it's necessary because Docker won't create that directory when we run docker-compose up because we don't have any binds that begin with wp-content/uploads:

`${ source.path }:/var/www/html/wp-content/plugins/${ source.basename }`,
// If this is is the Gutenberg plugin, then mount its E2E test plugins.
// TODO: Implement an API that lets Gutenberg mount test plugins without this workaround.
...( fs.existsSync( path.resolve( source.path, 'gutenberg.php' ) )
? [
`${ source.path }/packages/e2e-tests/plugins:/var/www/html/wp-content/plugins/gutenberg-test-plugins`,
`${ source.path }/packages/e2e-tests/mu-plugins:/var/www/html/wp-content/mu-plugins`,
]
: [] ),
] );
const themeMounts = config.themeSources.map(
( source ) =>
`${ source.path }:/var/www/html/wp-content/themes/${ source.basename }`

@talldan
Copy link
Contributor

talldan commented Feb 24, 2020

Okey dokey. It indeed didn't make any difference.

If you have any insights into why uploads might not be working in those tests that'd be great.

Copy link
Contributor

@talldan talldan left a comment

Choose a reason for hiding this comment

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

I tested this and it solves the issue, code looks good too (only a minor doc comment). Nice work 👍

*
* See https://github.com/docker-library/wordpress/issues/436.
*
* @param {string} environment The environment to check. Either 'development' or 'tests'.
Copy link
Contributor

Choose a reason for hiding this comment

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

Minor suggestion. I've had the feedback before to document this slightly differently, I think since this was merged:
https://github.com/WordPress/gutenberg/pull/18920/files#diff-78efab576563261e860347a820aa0997R234

You can inline a bit like this or I think consider a typedef if alignment gets difficult.

Suggested change
* @param {string} environment The environment to check. Either 'development' or 'tests'.
* @param {'development'|'tests'} environment The environment to check.

Copy link
Member Author

Choose a reason for hiding this comment

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

Good idea! This exists in a few other places and I can spot a few other JSDoc mistakes so I'll tackle this separately.

@noisysocks noisysocks merged commit b2ed0d6 into master Feb 28, 2020
@noisysocks noisysocks deleted the fix/wp-env-uploads-fail-on-production-wordpress branch February 28, 2020 03:14
@github-actions github-actions bot added this to the Gutenberg 7.7 milestone Feb 28, 2020
@jorgefilipecosta
Copy link
Member

Hi @noisysocks should this PR be backported into WordPress 5.4? If yes, feel free to add the label backport to wp core.

@noisysocks
Copy link
Member Author

I don't think we use @wordpress/env in Core. It's definitely not in Core's package.json. I'd expect that this fix is released as part of @wordpress/env 1.1 the next time packages are published

@jorgefilipecosta jorgefilipecosta added the Backport to WP 6.7 Beta/RC Pull request that needs to be backported to the WordPress major release that's currently in beta label Mar 2, 2020
jorgefilipecosta pushed a commit that referenced this pull request Mar 2, 2020
Makes the WordPress content directories (wp-content, wp-content/plugins,
wp-content/themes) owned by the www-data user. This ensures that WordPress
can write to these directories.

This is necessary when running wp-env with `"core": null` because Docker
will automatically create these directories as the root user when binding
volumes during `docker-compose up`, and `docker-compose up` doesn't support
the `-u` option.

See docker-library/wordpress#436.
jorgefilipecosta pushed a commit that referenced this pull request Mar 2, 2020
Makes the WordPress content directories (wp-content, wp-content/plugins,
wp-content/themes) owned by the www-data user. This ensures that WordPress
can write to these directories.

This is necessary when running wp-env with `"core": null` because Docker
will automatically create these directories as the root user when binding
volumes during `docker-compose up`, and `docker-compose up` doesn't support
the `-u` option.

See docker-library/wordpress#436.
@jorgefilipecosta jorgefilipecosta removed the Backport to WP 6.7 Beta/RC Pull request that needs to be backported to the WordPress major release that's currently in beta label Mar 3, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Tool] Env /packages/env [Type] Bug An existing feature does not function as intended
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants