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

Multi page types #16

Open
wants to merge 14 commits into
base: 1.x
Choose a base branch
from
Open

Multi page types #16

wants to merge 14 commits into from

Conversation

rupertj
Copy link
Member

@rupertj rupertj commented May 24, 2024

Implements #2

@@ -69,5 +71,40 @@ function localgov_subsites_extras_entity_bundle_create($entity_type_id, $bundle)
$type->setThirdPartySetting('menu_ui', 'available_menus', $availableMenus);
$type->save();
}
}

// phpcs:disable
Copy link
Contributor

Choose a reason for hiding this comment

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

Isn't it better to make the code meet Drupal standards?

Copy link
Member Author

@rupertj rupertj May 24, 2024

Choose a reason for hiding this comment

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

Drupal's code style rules don't let you implement hooks for other modules like I've done here, as they insist on the function prefix being the module name. This is just a temporary thing anyway, as I'd like to move those hooks to the modules that provide those types & fields.

Choose a reason for hiding this comment

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

Just taking a quick look, would an event and event subscriber be better here? And then the module can provide appropriate events for known localgov drupal content types until we can agree on a place to move it.

Copy link
Member Author

Choose a reason for hiding this comment

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

I've moved the hooks to their own file. Putting them there instead of in the module means that phpcs doesn't enforce the "function names must start with the module name rule", so I can re-enable phpcs on them.

I'm not against moving from hooks to an event. I don't think it's obviously better though. What do you reckon @Polynya?

@finnlewis
Copy link
Member

Discussing briefly in Merge Tuesday, sounds like a bit more testing might be needed before approving. I'll try to test soon.

@Polynya
Copy link
Contributor

Polynya commented May 28, 2024

I haven't had chance to test this yet but can you change a line in walkMenuTree like this:

return $parentNode ? $this->walkMenuTree($parentNode) : NULL;

This should prevent an error if a node has a menu parent that is not a node, e.g. events below the listing page.

See https://github.com/localgovdrupal/localgov_subsites_extras/pull/14/files#diff-0bc0657a906fb9cf8b5668cf07d0fc36e4c3fa1a8dabece778c28fffd53d3335R113

@rupertj
Copy link
Member Author

rupertj commented May 28, 2024

I haven't had chance to test this yet but can you change a line in walkMenuTree like this: ...

Good spot. I've added that.

… so we don't have to ignore them for code style checks.
@rupertj rupertj changed the title Feature/1.x/2 multi page types Multi page types Jun 25, 2024
@markconroy
Copy link
Member

Let's get this merged as soon as possible, it looks like a really good feature to have.

@rupertj can you get the merge conflict sorted, and then @Polynya can you get it tested again?

Great work team!

@markconroy
Copy link
Member

Thanks @rupertj

@Polynya over to you to test again if you can please?

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

Successfully merging this pull request may close these issues.

None yet

6 participants