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

Fix getWPAdminUrl function to actually return the /wp-admin/ URL #7096

Closed
wants to merge 6 commits into from

Conversation

mkaz
Copy link
Member

@mkaz mkaz commented Jun 1, 2018

Description

This adds a getter and setter function to wp.utils for the wpAdminURL.

When loading Gutenberg, it will call setWPAdminURL( %s ) where %s is the results of admin_url()

This can then be used when needed with a similar call using wp.utils.getWPAdminURL( path )

Fixes #7095

How has this been tested?

The getWPAdminURL is used during "Move to Trash", and confirmed the proper admin url is returned during the trash sequence.

You can also check it is set properly by calling wp.utils.getWPAdminURL( 'foo.php' ) in the console after a page is loading and it should return a full /wp-admin/foo.php however it is configured for the WP install.

@@ -23,7 +23,8 @@ export function getPostEditUrl( postId ) {
* @return {string} WPAdmin URL.
*/
export function getWPAdminURL( page, query ) {
return addQueryArgs( page, query );
const url = ( window._wpAdminURL ) ? window._wpAdminURL + page : page;
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should avoid introducing new globals, instead, I'd personally prefer something like we're doing in #7018 :

A config function setWPAdminUrl() and call this function using wp_add_inline_script when registering the wp-utils script on the server.

@mkaz
Copy link
Member Author

mkaz commented Jun 2, 2018

@youknowriad Agreed, I didn't like the idea of another global either.

I noticed in a few other utils using select, which I wrongly assumed state may not be available in utils, but its editor/utils so already instantiated. I switched it over to use editorSettings, similar to how isRTL and others are set.

I thought since those settings are already in place, might be a bit simpler than new setter functions, let me now what you think. I can try the other approach if you prefer.

@mkaz
Copy link
Member Author

mkaz commented Jun 3, 2018

Failing test might show my initial assumption was right about state not being available.

@@ -1093,6 +1093,7 @@ function gutenberg_editor_scripts_and_styles( $hook ) {
'bodyPlaceholder' => apply_filters( 'write_your_story', __( 'Write your story', 'gutenberg' ), $post ),
'isRTL' => is_rtl(),
'autosaveInterval' => 10,
'wpAdminURL' => admin_url(),
Copy link
Member

Choose a reason for hiding this comment

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

Another challenge is that admin_url() is filterable, meaning it can be modified based on the provided path. I don't think this is a significant issue, but something to be aware of in the decision-making process.

It appears getWPAdminURL() is only used in a few places:

editor/utils/url.js
14:	return getWPAdminURL( 'post.php', { post: postId, action: 'edit' } );
25:export function getWPAdminURL( page, query ) {

editor/components/post-permalink/index.js
19:import { getWPAdminURL } from '../../utils/url';
112:						href={ getWPAdminURL( 'options-permalink.php' ) }

editor/components/post-last-revision/index.js
13:import { getWPAdminURL } from '../../utils/url';
19:				href={ getWPAdminURL( 'revision.php', { revision: lastRevisionId, gutenberg: true } ) }

editor/store/effects.js
27:import { getPostEditUrl, getWPAdminURL } from '../utils/url';
273:		window.location.href = getWPAdminURL( 'edit.php', {

Can we expose these individual URLs instead, and remove getWPAdminURL() entirely?

Copy link
Member Author

Choose a reason for hiding this comment

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

Exposing the URLs could be worthwhile, especially if it abstract a level higher.

Because even with this fix it doesn't really solve the issue I'm trying to address, it fixes a bug and prevents some weird behavior after "Move to Trash" is called but what we really want is to define where the user is redirected. After trash event it is hard coded to edit.php which doesn't make that much sense in our P2/front-end use case.

I'm not sure how granular, or annoying, it might be to set all of these, for this example, something like "RedirectURLAfterTrash()"

Copy link
Member

Choose a reason for hiding this comment

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

Fair enough. Want to open a new issue for future consideration?

Copy link
Member Author

Choose a reason for hiding this comment

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

@danielbachhuber Created => #7136

@aduth
Copy link
Member

aduth commented Jun 4, 2018

Related (conflicting, moves getPostEditUrl out of editor/utils/url.js): #7122

At a high level, getWPAdminURL doesn't seem like it ought to be an editor-specific function. That the proposed changes here require core/editor to derive the value further digs us into this.

@mkaz mkaz added the [Status] In Progress Tracking issues with work in progress label Jun 4, 2018
@mkaz mkaz removed the [Status] In Progress Tracking issues with work in progress label Jun 4, 2018
@mkaz
Copy link
Member Author

mkaz commented Jun 4, 2018

@aduth @youknowriad - I think this is the change you were suggesting.
Take a look, I moved the function out of editor and to wp.utils

@aduth
Copy link
Member

aduth commented Jun 19, 2018

Will this be needed if #7228 is merged?

@mkaz
Copy link
Member Author

mkaz commented Jun 20, 2018

@aduth 🤷‍♂️ #7228 will solve the problem by being able to set the redirect URL for trashing a post, so for my specific case it bypasses the problem with a better solution.

However the getWPAdminUrl function will still not return the wp-admin url, but most use cases will be within the wp-admin context so likely not an issue, until it is. :-)

@gziolo gziolo modified the milestones: 3.1, 3.2 Jun 21, 2018
@mcsf mcsf modified the milestones: 3.2, 3.3 Jul 5, 2018
@mcsf
Copy link
Contributor

mcsf commented Jul 10, 2018

Closing per the discussion starting at #7095 (comment), but anyone up for it can reopen.

@mcsf mcsf closed this Jul 10, 2018
@mcsf mcsf deleted the fix/getWPAdminURL branch July 10, 2018 15:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
REST API Interaction Related to REST API
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants