Initial bundle setup & feature implementation #1

weaverryan · 2018-09-09T17:44:35Z

Hi guys!

This adds all the low-level details (composer, travis, etc) + the features themselves, which the README describes. Notes:

Bundle supports PHP 7.1 and above

Why this bundle?

An optional feature in Webpack 4 allows Webpack to "split" an entry (e.g. app.js) into multiple, smaller files for performance. But, this means that you need multiple <script> tags for each entry... but these are dynamically created and changing. To help with that, Encore dumps a new entrypoints.json file, which describes which JS (or CSS) files are needed for each entry. This bundle adds a feature to read those. That's described in the README below (I'll likely move most of the README into the official docs).

Cheers!

Nyholm

I think it looks good. Just found some minors

composer.json

Nyholm · 2018-09-09T17:49:17Z

src/Asset/EntrypointLookup.php

+
+    private $entriesData;
+
+    private $returnedFiles = [];


Or, we might be okey with [] since fabbot is not active here.

Yea, not sure. We use [] on MakerBundle, but were forced to not use fabbot due to this :/. Using fabbot is ideal.

no reason to use array() here, as there is no merge conflict to save us from

Nyholm · 2018-09-09T17:50:51Z

src/Asset/ManifestLookup.php

@@ -0,0 +1,46 @@
+<?php
+


Should we declare classes as strict?

I'm fine with that, but we're not using them in symfony/symfony yet - I'm not sure if that's a policy, or for other reasons (that don't apply here)

Personally, I don't see the benefit of strict mode: it provides no benefit for the "public" facing APIs.
Of course strict mode can help spot some errors inside the boundaries of the facing API, but anyway, this is covered by community work+tests.

@nicolas-grekas

it provides no benefit for the "public" facing APIs.

Doesn't it? Does it not trigger type errors if the API is misused from userland?

Nope, not any, see also discussion in symfony/symfony#28423

You're right, I misunderstood this comment. 🙈 Thanks!

triggering such error is what types are doing, not strict mode (strict mode in userland makes type stricter for arguments)

nicolas-grekas

random comments :)

.gitignore

nicolas-grekas · 2018-09-09T17:57:04Z

.travis.yml

+          # Test the latest stable release
+        - php: 5.5
+        - php: 5.6
+        - php: 7.0


not needed, minimum php is 7.1, isn't it?

nicolas-grekas · 2018-09-09T17:57:40Z

.travis.yml

+          # Test LTS versions. This makes sure we do not use Symfony packages with version greater
+          # than 3. Read more at https://github.com/symfony/lts
+        - php: 7.2
+          env: DEPENDENCIES="symfony/lts:^3"


going to be deprecated, better use global flex + SYMFONY_REQUIRE (see symfony's .travis.yml)

I should update our docs for "best practices for reusable bundles"

There's a LOT of custom code in symfony's .travis.yml file - I'm not sure what's needed. I see the install flex part - https://github.com/symfony/symfony/blob/master/.travis.yml#L198-L205 - I need that, makes sense. But what about the big "running tests" block? https://github.com/symfony/symfony/blob/master/.travis.yml#L216-L244

Yes, only L198-L205, the rest is too specific

README.md

nicolas-grekas · 2018-09-09T18:48:12Z

.gitignore

@@ -0,0 +1,3 @@
+composer.lock
+phpunit.xml
+vendor/


missing EOL (there are more in the PR)

How about adding a .editorconfig to the project? This would reduce these kinds of errors.

nicolas-grekas · 2018-09-09T18:49:46Z

.travis.yml

+          # Minimum supported dependencies with the latest and oldest PHP version
+        - php: 7.2
+          env: COMPOSER_FLAGS="--prefer-stable --prefer-lowest" SYMFONY_DEPRECATIONS_HELPER="weak_vendors"
+        - php: 7.0


nicolas-grekas · 2018-09-09T18:50:25Z

.travis.yml

+          # Test LTS versions. This makes sure we do not use Symfony packages with version greater
+          # than 3. Read more at https://github.com/symfony/lts
+        - php: 7.2
+          env: DEPENDENCIES="symfony/lts:^3"


Yes, only L198-L205, the rest is too specific

chalasr · 2018-09-09T18:51:39Z

src/Asset/ManifestLookup.php

+            }
+        }
+
+        return isset($this->manifestData[$path]) ? $this->manifestData[$path] : null;


return $this->manifestData[$path] ?? null;

Is that the same? Does your version not throw a warning about a non-existent key?

It does not, that's the same :)

chalasr · 2018-09-09T18:58:41Z

composer.json

+    "type": "symfony-bundle",
+    "license": "MIT",
+    "authors": [
+        {


You could add yourself, that's deserved IMHO

stof · 2018-09-10T06:58:29Z

README.md

+
+This bundle allows you to use the `splitEntryChunks()` feature
+from [Webpack Encore](https://symfony.com/doc/current/frontend.html)
+by reading an `entrypoins.json` file and helping you render all of


typo in the file name

src/Asset/EntrypointLookup.php

stof · 2018-09-10T07:40:18Z

src/Asset/TagRenderer.php

+    private function getAssetPath(string $assetPath, string $packageName = null): string
+    {
+        if (null === $this->packages) {
+            throw new \Exception('To render the script or link tags, run "composer require symfony/asset".');


All public APIs of this class are reaching this validation. So why is the Packages dependency an optional one in the constructor ?

Good question. And, there may be a better solution. It's so that I can have:

<argument type="service" id="assets.packages" on-invalid="null" />

For this service's definition. I suppose I could remove this service definition entirely in a compiler pass if assets.packages is not present. However, this TagRenderer service is itself required by the Twig extension. So it would need on-invalid="null" and to have some code to throw an exception in the correct situation. I'm not sure if one solution is absolutely better than another, or if there's a general standard.

or remove also the Twig extension when it does not have the needed dependency, which is what we do in symfony itself.

stof · 2018-09-10T07:43:02Z

src/Asset/TagRenderer.php

+        foreach ($this->entrypointLookup->getCssFiles($entryName) as $filename) {
+            $scriptTags[] = sprintf(
+                '<link rel="stylesheet" href="%s" />',
+                $this->getAssetPath($filename, $packageName)


this is missing HTML escaping.

You're absolutely right. But, this is tricky: the asset path needs the equivalent of html_attr escaping from Twig. However, this bundle currently does not rely on Twig, and the logic for this escaping is too big to copy (https://github.com/twigphp/Twig/blob/84b2eb541026dbd39590ba80269ae7676b1bfc46/lib/Twig/Extension/Core.php#L1033) and it's not a good idea anyways, since it's security-related.

The only solution I can think of is to couple the bundle to Twig (no huge deal), and either use that twig_escape_filter function directly (bit hacky) or render Twig templates inside of TagRenderer.

Any other ideas?

well, at first, you can still apply normal PHP escaping. As you put this inside a quoted HTML attribute, things are fine (most of the escaping performed by html_attr is to allow to use it outside quotes, so for attribute names or unquoted attribute values).

Not doing any escaping can lead to broken HTML here

Ah, ok. I've just used htmlentities and added a test. If you know of any issues with that, please let me know!

stof · 2018-09-10T07:43:05Z

src/Asset/TagRenderer.php

+        foreach ($this->entrypointLookup->getJavaScriptFiles($entryName) as $filename) {
+            $scriptTags[] = sprintf(
+                '<script src="%s"></script>',
+                $this->getAssetPath($filename, $packageName)


this is missing HTML escaping.

src/Asset/EntrypointLookup.php

src/Twig/EntryFilesTwigExtension.php

BackEndTea · 2018-09-14T07:42:06Z

composer.json

+    ],
+    "autoload": {
+        "psr-4": {
+            "Symfony\\WebpackEncoreBundle\\": "src"


Shouldn't there be a trailing / after src?

not needed. Composer would trim it if it exists.

BackEndTea · 2018-09-14T07:43:32Z

src/Asset/ManifestLookup.php

+            }
+
+            $this->manifestData = json_decode(file_get_contents($this->manifestPath), true);
+            if (0 < json_last_error()) {


What about making this JSON_ERROR_NONE !== json_last_error()? It would make it more clear what is happening

I agree here

weaverryan · 2018-10-02T05:06:50Z

Updated! In a few days (and after the tests are working), I'll merge this. So, any additional feedback is warmly appreciated :).

stof · 2018-10-16T14:20:13Z

This should wait for symfony/webpack-encore#410 which changes a bit the structure of the entrypoints file (CSS becomes a string rather than an array of strings)

stof · 2018-10-19T15:45:49Z

.php_cs.dist

+$finder = PhpCsFixer\Finder::create()
+    ->in(__DIR__.'/src')
+    // the PHP template files are a bit special
+    ->notName('*.tpl.php')


do we have such files ?

stof · 2018-10-19T15:46:05Z

.php_cs.dist

+
+if (\PHP_VERSION_ID < 70100) {
+    // EntityRegenerator works only with PHP 7.1+
+    $finder->notName('EntityRegenerator.php');


that looks wrong here. Bad copy-paste ?

stof · 2018-10-19T15:46:21Z

.php_cs.dist

+        'semicolon_after_instruction' => false,
+        'header_comment' => [
+            'header' => <<<EOF
+This file is part of the Symfony MakerBundle package.


stof · 2018-10-19T15:48:10Z

simple-phpunit

+
+error_reporting(-1);
+
+$getEnvVar = function ($name, $default = false) {


this file looks wrong. Install the phpunit-bridge instead of duplicating its code.

Temporary code - debugging a weird Travis-only phpunit issue :/

Nyholm · 2018-10-20T06:36:35Z

Since this is the first lines of code to repo, I suggest we should merge it now. It may not be perfect but it does not have to be perfect now.

If this is merged now we all can help to make it better.

👍 good job

This PR was merged into the master branch. Discussion ---------- Initial bundle setup & feature implementation Hi guys! This adds all the low-level details (composer, travis, etc) + the features themselves, which the README describes. Notes: * Bundle supports PHP 7.1 and above **Why this bundle?** An optional feature in Webpack 4 allows Webpack to "split" an entry (e.g. `app.js`) into multiple, smaller files for performance. But, this means that you need multiple `<script>` tags for each entry... but these are dynamically created and changing. To help with that, Encore dumps a new `entrypoints.json` file, which describes which JS (or CSS) files are needed for each entry. This bundle adds a feature to read those. That's described in the README below (I'll likely move most of the README into the official docs). Cheers! Commits ------- 6eecd88 Initial bundle setup & feature implementation

Nyholm approved these changes Sep 9, 2018

View reviewed changes

nicolas-grekas reviewed Sep 9, 2018

View reviewed changes

weaverryan mentioned this pull request Sep 9, 2018

Adding recipe for symfony/webpack-encore-bundle symfony/recipes#458

Merged

pierredup reviewed Sep 9, 2018

View reviewed changes

README.md Outdated Show resolved Hide resolved

nicolas-grekas reviewed Sep 9, 2018

View reviewed changes

chalasr reviewed Sep 9, 2018

View reviewed changes

stof requested changes Sep 10, 2018

View reviewed changes

yceruto reviewed Sep 10, 2018

View reviewed changes

src/Twig/EntryFilesTwigExtension.php Outdated Show resolved Hide resolved

BackEndTea reviewed Sep 14, 2018

View reviewed changes

stof requested changes Oct 19, 2018

View reviewed changes

Initial bundle setup & feature implementation

6eecd88

weaverryan force-pushed the first-implementation branch from 0ca2ab2 to 6eecd88 Compare October 21, 2018 18:56

weaverryan merged commit 6eecd88 into master Oct 21, 2018

weaverryan deleted the first-implementation branch October 21, 2018 18:57


		error_reporting(-1);

		$getEnvVar = function ($name, $default = false) {

Uh oh!

Initial bundle setup & feature implementation #1

Initial bundle setup & feature implementation #1

Uh oh!

Conversation

weaverryan commented Sep 9, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Nyholm left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nicolas-grekas Sep 9, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nicolas-grekas Sep 9, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nicolas-grekas left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chalasr Sep 11, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

weaverryan commented Sep 9, 2018 •

edited

Loading

nicolas-grekas Sep 9, 2018 •

edited

Loading

nicolas-grekas Sep 9, 2018 •

edited

Loading

chalasr Sep 11, 2018 •

edited

Loading