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

Add more hooks v2 #882

Closed
Sama34 opened this issue Jul 5, 2014 · 39 comments
Closed

Add more hooks v2 #882

Sama34 opened this issue Jul 5, 2014 · 39 comments
Assignees
Labels
b:1.8 Branch: 1.8.x s:resolved Status: Resolved. Solution implemented or scheduled t:enhancement Type: Enhancement. Contains minor improvements
Milestone

Comments

@Sama34
Copy link
Contributor

Sama34 commented Jul 5, 2014

Feel free to suggest necessary hooks.

If you push directly please do so on an one-commit one-hook format unless you feel it not to be necessary (i.e: one-commit one-file). Thanks!

@mybb/developers

@Sama34 Sama34 added this to the 1.8 Beta 3 milestone Jul 5, 2014
@Sama34 Sama34 self-assigned this Jul 5, 2014
Sama34 pushed a commit that referenced this issue Jul 5, 2014
> so that we can modify specific portions of the thread items before it
is output.

**Reference:** http://community.mybb.com/thread-122419.html
Sama34 pushed a commit that referenced this issue Jul 5, 2014
> I think one also needs adding above the query to get the threads; the
amount of times I want to alter something here and it proves almost
impossible.

**Reference:**
http://community.mybb.com/thread-122419-post-884062.html#pid884062
@Starpaul20
Copy link
Member

We should add a hook somewhere around the login function of the Admin CP.

Also another place hooks should be added is at the end of the various forms in the Admin CP. This would make it much easier to add to those forms. The hooks could be placed just before $form_container->end(); in the Admin files.

Sama34 pushed a commit that referenced this issue Jul 5, 2014
Sama34 pushed a commit that referenced this issue Jul 5, 2014
Update usercp2.php

_usercp2_end_ was never to be run.
Sama34 pushed a commit that referenced this issue Jul 5, 2014
_syndication_get_posts_ to syndication.php
Sama34 pushed a commit that referenced this issue Jul 6, 2014
Sama34 pushed a commit that referenced this issue Jul 6, 2014
Update usercp2.php

_usercp2_end_ was never to be run.
@Sama34
Copy link
Contributor Author

Sama34 commented Jul 6, 2014

@PaulBender

We should add a hook somewhere around the login function of the Admin CP.

What about two in the show_login() method? We can as well change print and echo to a variable.

Also another place hooks should be added is at the end of the various forms in the Admin CP. This would make it much easier to add to those forms. The hooks could be placed just before $form_container->end(); in the Admin files.

But one is already found inside those methods. Would it be really neccessary to add that many new hooks?

@Starpaul20
Copy link
Member

What about two in the show_login() method? We can as well change print and echo to a variable.

That sounds good. Maybe we can also add some hooks in the new login datahandler too.

But one is already found inside those methods. Would it be really neccessary to add that many new hooks?

I've never been able to get the $form_container->end(); hook to work when I want it to.

Sama34 pushed a commit that referenced this issue Jul 6, 2014
Add hooks to the login data-handler.
Sama34 pushed a commit that referenced this issue Jul 6, 2014
Fix hooks in the login data-handler..
@Destroy666x
Copy link
Contributor

In my opinion:

  • hook in global.php before any templates but with access to all most necessary variables. Currently you can't for example use $theme in a code that should work in header template easily. Neither with global_start ($theme not defined), nor with global_end (header already eval'd).
  • ACP -> Edit User etc. start hook can't use $user info easily because get_user() is after hook... I have honestly no clue why. Should switch their places.
  • more hooks in contact.php - when trying to send message (somewhere at the beginning of $mybb->request_method == "post") and when message has been successfully sent (somewhere at the end of $mybb->request_method == "post")
  • inc/functions_online.php -> build_wol_row() function

@Sama34
Copy link
Contributor Author

Sama34 commented Jul 6, 2014

@Destroy666x do you mean something like $plugins->run_hooks('global_middle')?

@Destroy666x
Copy link
Contributor

@Sama34, I guess so.

Sama34 pushed a commit that referenced this issue Jul 7, 2014
```show_login()``` hooks.
@Sama34
Copy link
Contributor Author

Sama34 commented Jul 7, 2014

@PaulBender

I've never been able to get the $form_container->end(); hook to work when I want it to.

Feel free to provide examples so I can see if it is necessary.

@Destroy666x can you provide examples where it would be useful? I can't really any case where the current hooks didn't worked.

Currently you can't for example use $theme in a code that should work in header template easily. Neither with global_start ($theme not defined), nor with global_end (header already eval'd).

Yes, you can. Use global_end and do a replacement for a string instead of using a variable. Similar to how the breadcrumb is inserted.

@Sama34
Copy link
Contributor Author

Sama34 commented Jul 7, 2014

@Destroy666x

ACP -> Edit User etc. start hook can't use $user info easily because get_user() is after hook... I have honestly no clue why. Should switch their places.

Indeed that is a wrong placement of a hook.

if($mybb->input['action'] == "edit")
{
    $plugins->run_hooks("admin_user_users_edit");

    $user = get_user($mybb->input['uid']);

    // Does the user not exist?
    if(!$user['uid'])
    {
        flash_message($lang->error_invalid_user, 'error');
        admin_redirect("index.php?module=user-users");
    }

    // ...
}

Should be:

if($mybb->input['action'] == "edit")
{
    $user = get_user($mybb->input['uid']);

    // Does the user not exist?
    if(!$user['uid'])
    {
        flash_message($lang->error_invalid_user, 'error');
        admin_redirect("index.php?module=user-users");
    }

    $plugins->run_hooks("admin_user_users_edit");

    // ...
}

All similar behavior should be fixed. member_profile_start in member.php should be run before $uid = $memprofile['uid'];.

All hooks should be run after basic confirmation code, etc.

Any thoughts? @mybb/developers.

Sama34 pushed a commit that referenced this issue Jul 7, 2014
contact.php needs more hooks.
@Sama34
Copy link
Contributor Author

Sama34 commented Jul 7, 2014

@Destroy666x

inc/functions_online.php -> build_wol_row() function

Any example of this being useful/necessary? I hope you understand there are places we can't simply add new hooks without thinking clear first. (:

@Destroy666x
Copy link
Contributor

@Sama34

Use global_end and do a replacement for a string instead of using a variable.

Well, that's why I said 'easily' in my comment. Inserting strings such as

<something>

and then replacing them looks rather dirty and confusing for me. It's not the biggest deal, but it should be changed in 2.0 if not 1.8 in my opinion. Also, it was suggested by other developers as well as I can see: http://community.mybb.com/thread-140337.html

inc/functions_online.php -> build_wol_row() function Any example of this being useful/necessary?

Never mind, I actually thought about moving $plugins->run_hooks("online_user"); there, but now that I look at while loop in online.php and that function, I can't really remember why as it won't make almost any difference (maybe only if someone wants to use build_wol_row() in a plugin).

BTW, does anyone maintain a list of already moved/added hooks in 1.8?

@JordanMussi
Copy link
Contributor

@Destroy666x do you mean something like $plugins->run_hooks('global_middle')?

$plugins->run_hooks('global_templates') would be better IMO.

BTW, does anyone maintain a list of already moved/added hooks in 1.8?

We can run a diff of the file produced by https://github.com/euantorano/MyBB-Hook-Finder (it wont be too hard to do post-release).

@ATofighi
Copy link
Contributor

I think Hooks should add to all of the functions.
And for example line 505 of functions.php is

$date = $plugins->run_hooks("my_date", $date);

I think it's bad And this code is good:

        $argments = array(
                            "date" => $date,
                            "format" => $format,
                            "stamp" => $stamp,
                            "offset" => $offset,
                            "ty" => $ty,
                            "adodb" => $adodb
                         );
        $date = $plugins->run_hooks("my_date", $argments);

@JN-Jones
Copy link
Contributor

We can't add hooks to every function... That'd be wasted code as you normally don't need to change the behavior of a function.

And for the second: you can globalize all of the variables, no need to add them as parameters.

@ATofighi
Copy link
Contributor

@JN-Jones But I can't use global for this variables because their only in the my_date function.
I test it.
Please see this code:

<?php
// Disallow direct access to this file for security reasons
if(!defined("IN_MYBB"))
{
    die("Direct initialization of this file is not allowed.<br /><br />Please make sure IN_MYBB is defined.");
}

$plugins->add_hook("my_date", "mydate");


function mydate_info()
{

    return array(
        "name"          => "My Date Test",
        "description"   => "",
        "website"       => "http://www.mybb.com",
        "author"        => "AliReza_Tofighi",
        "authorsite"    => "http://www.mybb.com",
        "version"       => "1.0",
        "guid"          => "",
        "compatibility" => "*"
    );
}

function mydate($d)
{
    global $format;
    echo $format;
    exit;
    return $d;
}

?>

@Sama34
Copy link
Contributor Author

Sama34 commented Jul 12, 2014

@ATofighi it would be better to add them to the global scope than modifying a hook, that could potentially break plug-ins.

Why would you need all those variables for?

Sama34 pushed a commit that referenced this issue Jul 14, 2014
Adds one more hook in global.php
@Sama34
Copy link
Contributor Author

Sama34 commented Jul 14, 2014

I added global_intermediate hook in global.php.

@mybb/developers

Sama34 pushed a commit that referenced this issue Jul 14, 2014
admin/modules/user/mass_mail.php
@Sama34
Copy link
Contributor Author

Sama34 commented Jul 14, 2014

329421b @PaulBender that was an interesting one, you think that is good enough?

@Starpaul20
Copy link
Member

Yes, that looks good

@Sama34
Copy link
Contributor Author

Sama34 commented Jul 17, 2014

What about xmlhttp.php? @mybb/developers.

@Destroy666x
Copy link
Contributor

Were the ACP Edit User etc. hooks moved (see my first comment here)? If not, will they be moved?

@Sama34
Copy link
Contributor Author

Sama34 commented Jul 17, 2014

They should, however I got not more feedback. There are plenty of places were hooks should be moved a few lines that shouldn't cause issues.

@Sama34
Copy link
Contributor Author

Sama34 commented Jul 17, 2014

I think one should be added in showteam.php (the page it self could get some improvements, but there is no time for it) after:

        else
        {
            $user['lastvisit'] = my_date('relative', $user['lastactive']);
        }

        $bgcolor = alt_trow();

But I'm not that sure either.

@Destroy666x
Copy link
Contributor

Well, there are 2 vs 0, so if noone has anything against it, I guess they can be moved. I can't see any potential danger.

I agree with showteam foreach($usergroup['user_list'] as $user){} hook. Either ^there or just anywhere in that loop.

@euantorano
Copy link
Member

Wish this had been done as a pull request rather than direct commits. That way I could track what had been added and see if there was any I thought was missing...

@DiogoParrinha
Copy link
Contributor

Guys if there's one or two we left out, we can always add later, don't worry about that. If it's done, we can simply close this and move on.

@Sama34
Copy link
Contributor Author

Sama34 commented Jul 18, 2014

@euantorano pretty sure every commit referenced here equals to one hook/file.

@PirataNervo I would suggest to close this at the last moment.

Sama34 pushed a commit that referenced this issue Jul 18, 2014
xmlhttp.php
Sama34 pushed a commit that referenced this issue Jul 18, 2014
Fixes ACP hooks positions.
@Sama34
Copy link
Contributor Author

Sama34 commented Jul 18, 2014

Ok, I have moved/added some ACP hooks, I can't fix everything (I hesitated to edit some). I really can't understand the logic behind all hooks in the ACP. Looks like they were added automatically really...

Sama34 pushed a commit that referenced this issue Jul 18, 2014
showteam.php
@DiogoParrinha
Copy link
Contributor

Alright I'm closing this then, thank you for your work.

Sama34 pushed a commit that referenced this issue Jul 20, 2014
Minor improvements.
@Sama34
Copy link
Contributor Author

Sama34 commented Jul 20, 2014

I did some minor modifications. I doubt that will break any plug-in.

Sama34 pushed a commit that referenced this issue Jul 21, 2014
@martec
Copy link
Contributor

martec commented Jul 24, 2014

@Sama34
i know now is late...
but if possible could not add hook to codebuttons template?
i trying make plugin of this http://community.mybb.com/thread-155941.html , but i don´t see any hook that i can make something...

if possible something in line 2781 in functions.php and line 847 of class_page.php without arguments

thanks

@Destroy666x
Copy link
Contributor

@martec, agree

@Shade-
Copy link
Contributor

Shade- commented Jul 28, 2014

I think you should also add an hook here: https://github.com/mybb/mybb/blob/feature/inc/datahandlers/post.php#L1219

Handling automatic subscriptions is hard for third party plugins, an hook should help.

@Sama34
Copy link
Contributor Author

Sama34 commented Jul 29, 2014

So that notification centers can hook in there?

@mybb/developers

@HellaMadMax
Copy link

Could a hook be added somewhere in here?
https://github.com/mybb/mybb/blob/master/inc/class_parser.php#L547

I want to do my own way of handling message filtering but I can't do that without editing files, since other files call this function directly.

@Sama34
Copy link
Contributor Author

Sama34 commented Aug 9, 2014

@iSnipeu I'm not sure about that.

@mybb/developers

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
b:1.8 Branch: 1.8.x s:resolved Status: Resolved. Solution implemented or scheduled t:enhancement Type: Enhancement. Contains minor improvements
Projects
None yet
Development

No branches or pull requests