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

Questions relating to old extension hooks. #102

Open
patricknelson opened this issue Apr 21, 2016 · 5 comments

Comments

@patricknelson
Copy link
Contributor

@patricknelson patricknelson commented Apr 21, 2016

This relates loosely to my older issue (#90) and PR (#91). I would like to discuss the existence updateGoogleSitemaps and updateGoogleSitemapItems extension hook. Currently the functionality here is such that these will operate on the final results coming from->getSitemaps() and ->getItems() but it's only called in the controller. While both of those core methods do already tie into the newer alterDataList extension hook, it may still make sense to keep them around since they affect the results coming out to the page, so that's fine.

However, for consistency, shouldn't these be centralized and located instead inside those methods instead of only in the controller to ensure the output coming from those methods matches what you would see in the final template? Why alter this only in the controller?

@TamaEaston

This comment has been minimized.

Copy link

@TamaEaston TamaEaston commented Aug 10, 2016

@patricknelson - did you make any progress with this? I'd love to see some documentation/ examples around the extension hooks.

@patricknelson

This comment has been minimized.

Copy link
Contributor Author

@patricknelson patricknelson commented Aug 11, 2016

It depends on what you mean by "progress" @TamaEaston 😄 Sure, I made progress in that I solved my specific use case issues, for example I have a central ->alterDataList(...) hook where I ensure that you don't even list page types which have zero results for the current domain (since I have a custom implementation of a multi-domain website).

To clarify (now that you caused me to think about it a bit more) I think the core point to opening this issue was more to discuss if it made sense to:

  1. Eliminate just specifically updateGoogleSitemapItems, since now we have a new alterDataList hook (which directly handles exactly the same task but more centrally and less repetitively). I think it makes sense to retain the other hook (updateGoogleSitemapItems) as it still provides utility that is not precisely solved by just having alterDataList since those are two different lists with different contents.
  2. Migrate the other hook updateGoogleSitemaps to being triggered on the root GoogleSitemap class to help simplify things. Currently you (unfortunately) have to setup yet another extension just to affect the sitemaps listed in the controller even though it is entirely sourced from GoogleSitemap::inst()->getSitemaps() -- why extend a controller to alter that list when it's sourced there? I understand it's only currently used in the controller, but extending two different objects when it otherwise would be easy to simply extend GoogleSitemaps makes no sense.
  3. Possibly refactor GoogleSitemapExtension to GoogleSitemapPageExtension to better define it's use/purpose because if you do want to extend GoogleSitemap you cannot use the most intuitive name as it's already taken by the aforementioned page extension. Confusing, I know...

For example, if you utilized all of the extension hooks (like I currently do):

/**
 * IMPORTANT: Class name contains superfluous "Custom" word because "GoogleSitemapExtension" is
 * already setup in the module itself (see #3 above).
 */
class GoogleSitemapCustomExtension extends Extension {

    /**
     * @param   DataList    $dataList
     * @param   string  $class
     */
    public function alterDataList(DataList &$dataList, $class) {
        // alter $dataList as you see fit...
    }

}

class GoogleSitemapControllerExtension extends Extension {

    /**
     * @param ArrayList $sitemaps
     */
    public function updateGoogleSitemaps(ArrayList &$sitemaps) {
        // ... modify the root sitemaps that can be output to the front-end.
    }

    /**
     * @param   ArrayList   $items
     * @param   string      $class
     * @param   int         $page
     */
    public function updateGoogleSitemapItems($items, $class, $page) {
        // ... this doesn't make sense anymore due to alterDataList on the
        // aptly named GoogleSitemapCustomExtension above...
    }

}
@TamaEaston

This comment has been minimized.

Copy link

@TamaEaston TamaEaston commented Aug 11, 2016

@patricknelson - that is awesomeness above and beyond the call of duty. Thank you.

@patricknelson

This comment has been minimized.

Copy link
Contributor Author

@patricknelson patricknelson commented Aug 11, 2016

I like to be thorough!

@wilr

This comment has been minimized.

Copy link
Owner

@wilr wilr commented Aug 11, 2016

Patches welcome!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.