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

Removed unused variables #604

Merged
merged 7 commits into from
Jan 29, 2020

Conversation

kbrodej
Copy link
Contributor

@kbrodej kbrodej commented Dec 24, 2019

Fix for #545

@kbrodej kbrodej force-pushed the hotfix/remove-unused-variables-#545 branch from dd62230 to 56be162 Compare December 24, 2019 15:11
@@ -135,8 +134,7 @@ protected function getBestCandidate() {
foreach ($group_resolvers as $plugin_id) {
/** @var \Drupal\og\OgGroupResolverInterface $plugin */
if ($plugin = $this->pluginManager->createInstance($plugin_id)) {
$plugins[$plugin_id] = $plugin;

// $plugins[$plugin_id] = $plugin;
Copy link
Collaborator

Choose a reason for hiding this comment

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

this doesn't seem right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed in the next commit.

Copy link
Contributor

@pfrenssen pfrenssen left a comment

Choose a reason for hiding this comment

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

Thanks for working on this! This is looking good, except for one case: PHP_CodeSniffer cannot understand the way that our test is using a variable assignment to trigger an exception. Instead of adding a fake line that returns the value, we should just disable this coding standards check for this case.

tests/src/Unit/PermissionEventTest.php Outdated Show resolved Hide resolved
@pfrenssen pfrenssen merged commit 76b0b6c into Gizra:8.x-1.x Jan 29, 2020
@pfrenssen pfrenssen mentioned this pull request Jan 29, 2020
3 tasks
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

3 participants