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

GH-55 Improve extension integration with PHPStan #71

Closed
wants to merge 3 commits into from

Conversation

mglaman
Copy link
Owner

@mglaman mglaman commented May 10, 2019

Fixes #55

@mglaman mglaman force-pushed the gh55-improve-phpstan-integration branch from 9cd79ee to 33261f4 Compare June 11, 2019 14:59
@attrib
Copy link
Contributor

attrib commented Jun 12, 2019

I'm currently testing this branch with drupal upgrade_status.

The first issue I run into is Fatal error: Cannot declare class Drupal, because the name is already in use in /Users/karl.fritsche/workspace/drupal_contrib/drupal/core/lib/Drupal.php on line 80

This is due to PHPStan\Drupal\Bootstrap::addCoreNamespaces() does a require to Drupal.php
When the code is run inside the Drupal Context this is already provided and fails with the above error message.

Changing it to a require_once works.

The first run passed in my case, but continue to review+test with the upgrade_status module.

@attrib
Copy link
Contributor

attrib commented Jun 12, 2019

Last comment + the branch would fix

https://www.drupal.org/project/upgrade_status/issues/3057853
https://www.drupal.org/project/upgrade_status/issues/3050653

From my testing this looks good, with the minor change posted above.

@mglaman
Copy link
Owner Author

mglaman commented Jun 12, 2019

@attrib We should not need to require Drupal.php per https://github.com/drupal/core/blob/8.6.x/composer.json#L216.

🤔 or does this change the operation of the loaded autoloading files.

$builder->parameters['drupalServiceMap'][$serviceId] = $serviceDefinition;
}
}
}

protected function camelize(string $id): string
Copy link
Contributor

Choose a reason for hiding this comment

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

This method is still used in loadConfiguration() (1/2 issues where travis complains)

Copy link
Contributor

Choose a reason for hiding this comment

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

Code can stay removed, if the unreachable code above is removed.


public function register(): void
private function registerServiceMap()
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
private function registerServiceMap()
private function registerServiceMap(): void

Return typehint missing (2/2 where travis complains)

@@ -95,6 +75,12 @@ public function loadConfiguration(): void
}
}
}
return;
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this correct? It seems to work, but all code after this is unreachable.

Copy link
Contributor

Choose a reason for hiding this comment

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

Got it now, code was moved to Bootstrap.php, code after return should be removed then.

@attrib
Copy link
Contributor

attrib commented Jun 12, 2019

@attrib We should not need to require Drupal.php per https://github.com/drupal/core/blob/8.6.x/composer.json#L216.

🤔 or does this change the operation of the loaded autoloading files.

Was this maybe added for drupal-check then?

@mglaman
Copy link
Owner Author

mglaman commented Dec 5, 2019

Refactoring this in #91

@mglaman mglaman closed this Dec 5, 2019
@mglaman mglaman deleted the gh55-improve-phpstan-integration branch December 15, 2019 16:41
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.

Improve the extension
2 participants