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

Restructure PHP code into separate files #985

Merged
merged 7 commits into from
Jun 5, 2017
Merged

Conversation

nylen
Copy link
Member

@nylen nylen commented Jun 1, 2017

This PR restructures all PHP code from index.php into lib/*.php files. (I am not sold on the lib/ name and open to suggestions there; I also considered php/*.php but this seemed worse somehow.)

This restructuring is first something we should do anyway (our index.php is getting quite long) and a first step towards addressing asset bundling for the plugin release process, as discussed at #953.

In a follow-up PR, the next step for the plugin release process will be to allow the release version of the plugin to contain a different index.php file from what we run in development mode, which will change the behavior of the script + stylesheet bundling code.

Beyond rearranging the PHP code into separate files, there are very few other changes here:

  • Add gutenberg_dir_path and gutenberg_url functions that call plugin_dir_path and plugins_url respectively with the appropriate arguments.
  • Add file doc comments for phpcs.

@nylen nylen requested review from mtias and ellatrix June 1, 2017 23:08
@nylen
Copy link
Member Author

nylen commented Jun 1, 2017

I think this is pretty straightforward, and would probably be comfortable merging it in the next couple days so that I can move forward with this in a separate PR:

allow the release version of the plugin to contain a different index.php file from what we run in development mode, which will change the behavior of the script + stylesheet bundling code.

Still, review would be welcome.

@westonruter
Copy link
Member

While you're at it, what about renaming index.php to something a bit more normal for a plugin, like gutenberg.php?

@mtias
Copy link
Member

mtias commented Jun 2, 2017

Do you think we could model it as if we were to place it in wp-includes?

@mtias mtias added the Framework Issues related to broader framework topics, especially as it relates to javascript label Jun 2, 2017
@nylen
Copy link
Member Author

nylen commented Jun 2, 2017

Do you think we could model it as if we were to place it in wp-includes?

Do you have a specific idea for how to work towards this now?

Otherwise, I think this would be better left to a separate set of tasks, because there is a lot to do. For example, some of this code will live in wp-includes, some will probably live in wp-admin, and we will have to change all the places in the code that get URLs and file paths.

@mtias
Copy link
Member

mtias commented Jun 2, 2017

Right, I mean mostly just the names of the files themselves if there are any patterns we should be following.

@nylen nylen force-pushed the update/restructure-php-code branch from 2f2c6fb to 5c7d641 Compare June 2, 2017 14:30
@nylen
Copy link
Member Author

nylen commented Jun 2, 2017

Right, I mean mostly just the names of the files themselves if there are any patterns we should be following.

I think we will end up with some files named gutenberg.php in various directories, and some of our code merged into existing files.

@nylen
Copy link
Member Author

nylen commented Jun 2, 2017

While you're at it, what about renaming index.php to something a bit more normal for a plugin, like gutenberg.php?

This is the recommended plugin folder structure. Unfortunately, this means that anyone using the plugin will get an error when they upgrade with this change, then attempt to load the editor:

Sorry, you are not allowed to access this page.

The fix is to visit the Plugins tab in wp-admin, where you will see this error:

The plugin gutenberg/index.php has been deactivated due to an error: Plugin file does not exist.

and then re-activate the plugin.

If we are going to do this change (it seems good to follow the recommended folder structure), we should do it now, before we release to the plugin directory. At this time it will mostly affect those of us who are developing the plugin.

Thoughts?

@nylen nylen force-pushed the update/restructure-php-code branch from 8d0367b to 750a14d Compare June 2, 2017 14:50
*/

if ( ! defined( 'ABSPATH' ) ) {
die( 'Silence is golden.' );
Copy link
Member

@ellatrix ellatrix Jun 2, 2017

Choose a reason for hiding this comment

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

This is kind of a paradox, if you're actually outputting it. :)

I usually do defined( 'ABSPATH' ) || die; to keep it short.

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 code should generate paradoxes once in a while.

@westonruter
Copy link
Member

westonruter commented Jun 3, 2017

If we are going to do this change (it seems good to follow the recommended folder structure), we should do it now, before we release to the plugin directory. At this time it will mostly affect those of us who are developing the plugin.

👍 Yes, the number of people who have the plugin installed once it is in the directory will (hopefully) increase by magnitude(s).

Unfortunately, this means that anyone using the plugin will get an error when they upgrade with this change, then attempt to load the editor

I think I have a fix for this in 750a14d..e949610, although I only tried it with switching branches and not using the plugin updater.

@nylen
Copy link
Member Author

nylen commented Jun 5, 2017

I think I have a fix for this in 750a14d..e949610, although I only tried it with switching branches and not using the plugin updater.

Thanks! This fix works well. There is currently nothing to test here using the plugin updater:

  • WordPress won't replace an already existing plugin uploaded from a .zip.
  • We released a gutenberg-2017-05-24.zip file in Slack, and doing a plugin build and uploading it to a site causes a fatal error due to functions being redefined. I'll address this issue separately.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Framework Issues related to broader framework topics, especially as it relates to javascript
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants