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 a script to generate a list of hooks that defined or used in GLA #1573

Merged
merged 8 commits into from
Jul 7, 2022

Conversation

ianlin
Copy link
Member

@ianlin ianlin commented Jun 23, 2022

Changes proposed in this Pull Request:

This PR adds a script to generate a list of hooks (actions or filters) that is defined or used in this project. The output file is generated in src/Hooks/README.md.

The majority of the codes were copied fro WooCommerce Code Reference repo, and they're published in https://woocommerce.github.io/code-reference/hooks/hooks.html.

Since the hooks in GLA project are mostly used internally so generated as a markdown file would be good enough.

Detailed test instructions:

  1. Run php bin/HooksDocsGenerator.php
  2. See if the script generated a markdown file in src/Hooks/README.md

Changelog entry

Add - A script to generate a list of hooks that defined or used in GLA.

@ianlin ianlin added the type: enhancement The issue is a request for an enhancement. label Jun 23, 2022
@ianlin ianlin requested a review from a team June 23, 2022 09:41
@ianlin ianlin self-assigned this Jun 23, 2022
Copy link
Contributor

@jorgemd24 jorgemd24 left a comment

Choose a reason for hiding this comment

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

Thanks, @ianlin for adding this, I have tested locally and I see that is creating the hooks docs correctly ✅ It would be perfect if we have a description for each hook (similar to the tracking page) but I think in this case, it is not possible.

Just a question related to the docs generator: should we run the hooks docs generator everytime that we add a new hook? or can it be automatize?

$output = "# Hooks Reference\n\n";
$output .= "A list of hooks, i.e `actions` and `filters`, that are defined or used in this project.\n\n";

foreach ( $hook_list as $heading => $hooks ) {
Copy link
Contributor

Choose a reason for hiding this comment

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

💅

$heading could be removed as it is not used.

Suggested change: foreach ( $hook_list as $hooks ) {

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks, updated in b7a2dfc.

@ianlin
Copy link
Member Author

ianlin commented Jun 28, 2022

Thanks for the review @jorgemd24. I also updated the README.md file to include hooks documentation infor.

It would be perfect if we have a description for each hook (similar to the tracking page) but I think in this case, it is not possible.

Indeed, it would be very helpful but currently it's not possible unless we write down the hooks we used using PHPDoc, and see if we can extend PHPDoc for having the custom tags in order to auto generate it.

should we run the hooks docs generator everytime that we add a new hook? or can it be automatize?

I think for now we need to run the script every time someone adds a new hook, AFAIK the tracking doc needs to be run manually as well (correct me if I'm wrong). Here's a possible automation approach:

  1. Add Git pre-commit hooks to check if the current staged changes have code difference with keywords do_action, add_action, apply_filter, and add_filter.
    • Maybe run git diff --cached --word-diff -U1 | grep -E 'add_action|do_action|add_filter|apply_filter'
  2. If yes, run the script to regenerate hooks document, then commit the changes (without the newly generated doc).
  3. Run git add and git commit again to commit the generated doc.

In this PR we have the first version of GLA hooks documentation, I have created a follow-up issue #1576 for enhancing the GLA hooks documentation process.

@jorgemd24
Copy link
Contributor

@ianlin Thanks for opening the follow-up issue!

Maybe run git diff --cached --word-diff -U1 | grep -E 'add_action|do_action|add_filter|apply_filter'

Yeah that could work however I think we could remove add_action and add_filter from the grep command as the hooks generator is only checking for do_action and apply_filter, correct me if I am wrong.

@ianlin
Copy link
Member Author

ianlin commented Jun 30, 2022

@jorgemd24
I added some changes to get all the files for parsing hooks, as previously the script did not get all the files.

@ianlin ianlin requested a review from jorgemd24 June 30, 2022 06:49
@jorgemd24
Copy link
Contributor

Thanks, @ianlin for fixing this. I see that the rest of the hooks are now included.

It took me a while to see why it was not included in all the hooks but looking at the code again I see that in the original codebase the following part is trying to avoid duplications:

                $retrieved_files = (array) self::getFiles($pattern, $flags, $p . '/');
                foreach ($retrieved_files as $file) {
                    if (! in_array($file, self::$found_files)) {
                        $found_files[] = $file;
                    }
                }

https://github.com/woocommerce/code-reference/blob/331da252e94b3e6529d2d42f6636200ca30dcfe4/generate-hook-docs.php#L112-L117

As far as I understand, $retrieved_files is always included in self::$found_files because has already been processed by the function self::getFiles therefore $found_files will be always empty and return only the files from the same directory level but not from subfolders. Please, correct me if I am wrong.

I think this problem is less visible in the original codebase because the script is including the subpaths:

        $files['Core Classes']       = array_merge(
            self::getFiles('*.php', GLOB_MARK, self::SOURCE_PATH . 'includes/'),
            self::getFiles('*.php', GLOB_MARK, self::SOURCE_PATH . 'includes/abstracts/'),
            self::getFiles('*.php', GLOB_MARK, self::SOURCE_PATH . 'includes/customizer/'),
            self::getFiles('*.php', GLOB_MARK, self::SOURCE_PATH . 'includes/emails/'),
            self::getFiles('*.php', GLOB_MARK, self::SOURCE_PATH . 'includes/export/'),
            self::getFiles('*.php', GLOB_MARK, self::SOURCE_PATH . 'includes/gateways/'),
            self::getFiles('*.php', GLOB_MARK, self::SOURCE_PATH . 'includes/import/'),
            self::getFiles('*.php', GLOB_MARK, self::SOURCE_PATH . 'includes/shipping/')
        );

I compared some hooks from the WC plugin and https://woocommerce.github.io/code-reference/hooks/hooks.html and I saw that some hooks are not included, for example:

Should this be reported to the WC team?

At the moment we do not have any hooks in the file but this could change in the future.

@ianlin
Copy link
Member Author

ianlin commented Jul 4, 2022

Hey @jorgemd24, sorry for not explaining the reason behind. You're right, the $found_files will be empty in the case where there are more than two levels of paths. For example, the original code will work on the structure like this:

src
├── Level-1/
│   └── level-2.php
└── level-1.php

However, it won't work on the structure like this:

src
├── Level-1/
│   ├── Level-2/
│   │   └── level-3.php
│   └── level-2.php
└── level-1.php

The reason is, level-3.php was already stored in self::found_files in the recursive call. When the call is going back to the top, although the $retrieved_files contains both level-2.php and level-3.php, the $found_files will only have level-2.php because level-3.php is treated as duplicated.

Should this be reported to the WC team?

Yeah, I think it's a good idea we report them this potential problem, I will create an issue on their repo then.

At the moment we do not have any hooks in the file but this could change in the future.

Not sure what do you mean about this?

@jorgemd24
Copy link
Contributor

Yeah, I think it's a good idea we report this potential problem, I will create an issue on their repo then.

Thanks!

At the moment we do not have any hooks in the file but this could change in the future.
Not sure what do you mean about this?

Sorry! I think I skipped part of the message 😄 . What I meant was that we are not processing the google-listings-and-ads.php file, that's fine for now, as we do not have any do_action or apply_filter, but maybe in the future, we would like to include it as well.

Similar like it is done here: https://github.com/woocommerce/code-reference/blob/331da252e94b3e6529d2d42f6636200ca30dcfe4/generate-hook-docs.php#L45

@ianlin
Copy link
Member Author

ianlin commented Jul 6, 2022

@jorgemd24

What I meant was that we are not processing the google-listings-and-ads.php file

Ah, thanks for catching this, added in 3da1be1.

Just created an issue woocommerce/code-reference#20 for the WC folks.

@jorgemd24
Copy link
Contributor

Thanks, @ianlin, I have added some hooks on google-listings-and-ads.php and I see that the hooks docs are updated. LGTM 👍

@ianlin ianlin merged commit 21263ae into develop Jul 7, 2022
@ianlin ianlin deleted the add/hooks-docs-generator branch July 7, 2022 03:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: enhancement The issue is a request for an enhancement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants