I believe $sandwich_plugin_info should be passed by reference for it to actually work. The current code modifies a copy, and I'm pretty certain that won't actually do anything.

Current code (plugin_type_example/plugin_type_example.api.php):

/**
 * Alter the definitions of all the Sandwich plugins.
 *
 * You can implement this hook to do things like change the properties for each
 * plugin or change the implementing class for a plugin.
 *
 * This hook is invoked by SandwichPluginManager::__construct().
 *
 * @param array $sandwich_plugin_info
 *   This is the array of plugin definitions.
 */
function hook_sandwich_info_alter(array $sandwich_plugin_info) {
  // Let's change the 'foobar' property for all sandwiches.
  foreach ($sandwich_plugin_info as $plugin_id => $plugin_definition) {
    $sandwich_plugin_info[$plugin_id]['foobar'] = t('We have altered this in the alter hook');
  }
}

I've also verified in a few core api.php files that their plugin info alter hooks do pass the definition array by reference - so I think the code example is incorrect.

Patch on the way...

CommentFileSizeAuthor
#2 examples-plugin-type-example-info-alter-pbv-bug-2931559-2.patch1.07 KBchrisolof

Comments

chrisolof created an issue. See original summary.

chrisolof’s picture

chrisolof
he/him
Primary language English
commented
Title: $sandwich_plugin_info is passed by value (should be reference) » $sandwich_plugin_info should be passed by reference
Status: Active » Needs review
StatusFileSize
new examples-plugin-type-example-info-alter-pbv-bug-2931559-2.patch1.07 KB

Patch attached.

mile23’s picture

mile23
Primary language English
Location Seattle, WA
commented
Status: Needs review » Needs work

Thanks.

When I look at the page, I see "Ham, mustard, ROCKET, sun-dried tomatoes." So the plugin_type_example_sandwich_info_alter() hook is having an effect.

The patch changes the API documentation for the hook, and that's good.

However: PluginTypeExampleTest says this:

  /**
   * Test the plugin manager can be loaded, and the plugins are registered.
   *
   * @todo: Check the alter hook fired and changed a property.
   */
  public function testPluginExample() {

We don't have a test that the hook worked, so let's add one.

PluginTypeExampleTest::testPluginExample() really should be its own kernel test, but that's optional for this issue.

mile23’s picture

mile23
Primary language English
Location Seattle, WA
commented
Component: Other » Plugin Type Example

  • Mile23 committed ac223b4 on 8.x-1.x authored by chrisolof 
    Issue #2931559 by chrisolof: $sandwich_plugin_info should be passed by...
mile23’s picture

mile23
Primary language English
Location Seattle, WA
commented
Status: Needs work » Fixed

The actual hook implementation looks like this:

function plugin_type_example_sandwich_info_alter(&$sandwich_plugin_info) {

So we're already doing this in code. This is a documentation bug.

We still have a @todo that needs a follow-up in #3, and here it is: #2985705: Ensure hook_sandwich_info_alter() works

Updated the todo with a link, fixed on commit.

Thanks!

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.