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

Disallow the use of query strings #67

Merged
merged 15 commits into from
Apr 15, 2023

Conversation

IanDelMar
Copy link
Contributor

@IanDelMar IanDelMar commented Mar 2, 2023

This PR adds array shapes for the $args parameter to the functionsMap.php for

  • get_objects_in_term()
  • WP_List_Table::set_pagination_args()
  • WP_Post_Type::__construct()
  • WP_Taxonomy::__construct()
  • wp_widget_rss_form().

As already done with other functions in the WP stubs this disallows the use of query string for the functions listed above.

@szepeviktor
Copy link
Member

Thank you Ian!
Could you update the stubs file?

@herndlm
Copy link
Contributor

herndlm commented Mar 2, 2023

Does that mean that all those types are not in the stubs already I suppose? Just asking because I remember registering a post type recently and it did enforce the arg type with keys properly

@herndlm
Copy link
Contributor

herndlm commented Mar 2, 2023

Does that mean that all those types are not in the stubs already I suppose? Just asking because I remember registering a post type recently and it did enforce the arg type with keys properly

just checked, looks like I was using register_post_type() and not the constructor 👍

@szepeviktor
Copy link
Member

Veto? Anyone?

@IanDelMar
Copy link
Contributor Author

This needs to go into functionMap62.php as well, right?

@szepeviktor
Copy link
Member

szepeviktor commented Apr 15, 2023

This needs to go into functionMap62.php as well, right?

Yes.
John promised he will think about abolishing "62" files.

@szepeviktor
Copy link
Member

@IanDelMar I'm very glad that you continue with this PR.

@IanDelMar
Copy link
Contributor Author

The PR should be up to date now

@szepeviktor
Copy link
Member

Thank you!

@szepeviktor szepeviktor merged commit 4e1764c into php-stubs:master Apr 15, 2023
@IanDelMar IanDelMar deleted the query-strings branch April 16, 2023 08:15
@johnbillion
Copy link
Contributor

johnbillion commented Apr 19, 2023

@IanDelMar What's your use case for needing the shape of the args for WP_Post_Type::__construct() and WP_Taxonomy::__construct()? We've already got them for register_post_type() and register_taxonomy().

Having those shapes defined in this repo increases the maintenance burden and the chance that they get out of sync with the args in core.

The visitor.php class supports inherited args from other functions when the @see tag a "See" reference is used in the parameter description (see #49) so we can do this via WordPress core and then remove the $registerPostTypeArgsType and $registerTaxonomyArgsType definitions in this repo.

@IanDelMar
Copy link
Contributor Author

@johnbillion Maybe they are not needed. In fact this is not about the shapes at all but removing the string from $args

	/**
	 * Constructor.
	 *
	 * See the register_post_type() function for accepted arguments for `$args`.
	 *
	 * Will populate object properties from the provided arguments and assign other
	 * default properties based on that information.
	 *
	 * @since 4.6.0
	 *
	 * @see register_post_type()
	 *
	 * @param string       $post_type Post type key.
	 * @param array|string $args      Optional. Array or string of arguments for registering a post type.
	 *                                Default empty array.
	 */
	public function __construct( $post_type, $args = array() ) {
		$this->name = $post_type;


		$this->set_props( $args );
	}

Source: https://github.com/WordPress/wordpress-develop/blob/6.2/src/wp-includes/class-wp-post-type.php#L399-L419

If there's a better way to do this, I'm happy to learn!

@johnbillion
Copy link
Contributor

Got it. I'll take a look.

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

Successfully merging this pull request may close these issues.

None yet

4 participants