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

Improvements #17

Open
turtlepod opened this issue Feb 3, 2013 · 1 comment
Open

Improvements #17

turtlepod opened this issue Feb 3, 2013 · 1 comment

Comments

@turtlepod
Copy link
Contributor

just some suggestion, i'm not on my computer right now, so this is only some that i remember.

API Script

index.php
on basic_check we don't need to feed all the data to wp. just "slug", "version" and "package" is needed in transient. i'm not even sure "slug" is needed
(it's for plugins i can't remember what's the req for theme)

on plugin_information it's better to set external to true.
and not all html is allowed in sections. i''ll posted later list of allowed html in sections data. so this need to be sanitized and validate before sending or after recieving the data (or both, just to make sure)

packages.php
theme screenshot is not used, it sill use screenshot from the themes (so it's not needed)

in plugin sections tab, you did it wrong, we cannot create our own tab, the allowed tab are:

  • description
  • installation
  • faq
  • changelog (no space)
  • screenshots (no space)
  • other_notes (with underscore)

everything need to be simplified, you don't need multiple array in there when you array_shift-ed the package in index.php.

something like this might be better:

$packages['slug'] = array(
    '0.1.0' => array(
        'author' => '',
        //other data
        //version in not needed, already in array data
        //external is not needed, add it in index.php before sending
        //need better way to handle sections to easy section input, try file_get_contents or similar solutions.
    );
);

Plugin example

$request_string: don't need api-key, i don't think we use it anywhere.
unserialize: may be better to use maybeunserialize wp functions (it's better)
bad global naming.
not all plugin file name is the same as plugin folder name.

i'm not really sure, but in

$checked_data->response[$plugin_slug .'/'. $plugin_slug .'.php'] = $response;

there's a possibility that response is not set yet (?) since it's transient
so it's better to set it first.
when handling transient data (cause it's temporary data), it's better to assume that the data is not always available.

i guess that's it for today!
i'll try to read it more in depth once i have the time.

@jeremyclark13
Copy link
Owner

Thanks so much for these suggestions. Most of the code was taken from another package and I just kept it up to date with wp versions. I do believe at one time you could set an arbitrary section title. I haven't tested in the latest version. Also I think the screenshot url for themes was used for wpmu notifications st one time. I'll have to double check this again though.

Sanitizing the html befor display is a very good idea as well. Didn't think about the consequences if the api server is compromised it could send malware thru the plugin information screen.

Thanks again. I'll try to do a more in depth review of the WordPress update system myself.

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

No branches or pull requests

2 participants