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

Add PhpScoper compactor #31

Merged
merged 12 commits into from
Mar 4, 2018
Merged

Conversation

theofidry
Copy link
Member

@theofidry theofidry commented Jan 20, 2018

Closes humbug/php-scoper#49

  • Make it work
  • Fix the Composer autoloading
  • Cleanup
  • Doc

bin/box Outdated
@@ -26,8 +26,8 @@ define('BOX_PATH', dirname(__DIR__));

if ($uri = Phar::running()) {
require "$uri/vendor/autoload.php";
} elseif (class_exists('Extract')) {
require __DIR__.'/../vendor/autoload.php';
//} elseif (class_exists('Extract')) {
Copy link

@soullivaneuh soullivaneuh Jan 31, 2018

Choose a reason for hiding this comment

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

Commented code?! Booh! 👻

Copy link
Member Author

Choose a reason for hiding this comment

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

That's why it's a PoC :D

I was accidentally using humbug/php-scoper#117 which is not ready yet and was failing on this class because it couldn't find it

@theofidry
Copy link
Member Author

Status: requires more work on PHP-Scoper for now:

theofidry added a commit to humbug/php-scoper that referenced this pull request Feb 2, 2018
Reworked the configuration to make it easier to re-use: the command now
provides the globalNamespaceWhitelister ready for use as well as the
list of files with their contents.

As the handler has a lot of IO-bound dependencies, it is actually very
hard to re-use so it makes more sense to integrate it back to the
command.

This paves the way for box-project/box#31.
theofidry added a commit to theofidry/php-scoper that referenced this pull request Feb 2, 2018
Continuing some work for box-project/box#31. The Scoper now takes the file
contents as an argument. This allows the scoper to be used with
non-exitent files (e.g. virtual files) or files for which we are
modifying the content like in Box.

Another benefit is that it removes the Scopers dependency to the file
system layer.
theofidry added a commit to humbug/php-scoper that referenced this pull request Feb 2, 2018
Continuing some work for box-project/box#31. The Scoper now takes the file contents as an argument. This allows the scoper to be used with existent files (e.g. virtual files) or files for which we are modifying the content like in Box.

Another benefit is that it removes the Scopers dependency to the file system layer.
@theofidry theofidry force-pushed the feature/php-scoper branch 2 times, most recently from c6e4399 to 577509c Compare February 6, 2018 21:23
@theofidry theofidry mentioned this pull request Feb 15, 2018
@TomasVotruba
Copy link

This is exactly what I need 👍

bin/box Outdated
@@ -50,6 +50,24 @@ $app->run();
*/
function load_composer_class_loader(?string $dir): ClassLoader
{
if (file_exists($autoload = __DIR__.'/../../../autoload.php')) {
Copy link
Member Author

Choose a reason for hiding this comment

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

this could be greatly simplified in a separate PR I think

Copy link
Member Author

Choose a reason for hiding this comment

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

see #64

scoper.inc.php Outdated
return [
'patchers' => [
function (string $filePath, string $prefix, string $contents): string {
$file = 'vendor/justinrainbow/json-schema/src/JsonSchema/Constraints/Factory.php';
Copy link
Member Author

Choose a reason for hiding this comment

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

Do a PR to fix that at the root

Copy link
Member Author

Choose a reason for hiding this comment

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

Actually should no longer be necessary with the latest version of PHP-Scoper

src/Box.php Outdated
@@ -175,6 +177,22 @@ function ($file): string {
);
}

$cwd = getcwd();
Copy link
Member Author

Choose a reason for hiding this comment

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

obviously this is not good enough

src/Box.php Outdated
@@ -296,9 +314,13 @@ private function processContents(array $files): array
self::replacePlaceholders($placeholders, $contents)
);

(new Filesystem())->dumpFile($tmp.DIRECTORY_SEPARATOR.$local, $processedContents);
Copy link
Member Author

Choose a reason for hiding this comment

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

dumping the file shouldn't be done here

return $this->scoper->scope(
$file,
$contents,
'TODOAllowNullPrefix',
Copy link
Member Author

Choose a reason for hiding this comment

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

private static function retrievePhpScoperConfig(stdClass $raw, string $basePath): PhpScoperConfiguration
{
if (!isset($raw->{'php-scoper'})) {
$configFilePath = $basePath.DIRECTORY_SEPARATOR.self::PHP_SCOPER_CONFIG;
Copy link
Member Author

Choose a reason for hiding this comment

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

use the file system

@theofidry theofidry force-pushed the feature/php-scoper branch 2 times, most recently from 6039597 to 2eafeab Compare February 27, 2018 14:18
There is a bug with the Composer namespace:

- Either do not prefix any Composer related package
- Either add an e2e test with Composer in PHP-Scoper
@theofidry theofidry force-pushed the feature/php-scoper branch from 2eafeab to 5d2301a Compare March 3, 2018 22:51
@theofidry theofidry changed the title PoC: Add PhpScoper compactor Add PhpScoper compactor Mar 4, 2018
@theofidry theofidry merged commit 3995ac0 into box-project:master Mar 4, 2018
@theofidry theofidry deleted the feature/php-scoper branch March 4, 2018 14:42
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.

3 participants