Improvements to the phpserver router #10500

aredridel · 2017-08-11T13:52:53Z

handles versioned static assets
our own mime handling for those, since php's built in web server's mime table isn't accessible from userland
works with a magento instance set up with use-rewrites=1

Is this a useful amendment? I'm happy to fix this up with suggestions, too, as I intend to use it for my daily development.

magento-cicd2 · 2017-08-11T13:52:55Z

All committers have signed the CLA.

fooman · 2017-08-16T23:21:08Z

Thank you @aredridel for your contribution. I haven't used the phpserver myself but anything that makes Magento easier to develop with sounds very useful to me. Travis picked up some minor style nitpicks - would you mind taking a look at those.

@Flyingmana I believe you provided the initial implementation. Any chance you are able to take a look?

Flyingmana

That is a great addition and I like it a lot, that you found solutions to this problems.

At the moment I dont have something to test these, but from what I see the changes make sense and will be very useful for other users of the phpserver

Flyingmana · 2017-08-17T06:11:59Z

phpserver/router.php

-
-    $debug($path);
-    $debug($route);
+    $ext = @pathinfo($route)['extension'];


are there cases when pathinfo is throwing an error, or is it because the array access for 'extension' is sometimes not working?

It's because there's not always an extension.

Updated to not use @

Flyingmana · 2017-08-17T06:22:02Z

phpserver/router.php

+                'html' => 'text/html',
+            ];
+
+            $type = @$mimeTypes[$ext];


I believe its still better to check via isset() instead of just surpressing the error here.
maybe outdated reference: https://stackoverflow.com/questions/12733666/which-is-better-in-php-suppress-warnings-with-or-run-extra-checks-with-isse

Yeah. I prefer the shorter but not a problem to go the other way.

updated to use isset

Flyingmana · 2017-08-17T06:30:47Z

phpserver/router.php

+            if ($type) {
+                header("Content-Type: $type");
+            }
+            return readfile($file);


is the return of readfile intended? I dont know how php handles the returned number of bytes

Habit -- I always return the return value from things. Too long as a rubyist.

Separated the return

fooman · 2017-08-19T02:42:31Z

@aredridel thanks for making those changes. This all looks good to me and I have put this in the queue to be processed for merging into develop.

hostep · 2017-08-19T16:18:33Z

phpserver/router.php

+                'html' => 'text/html',
+            ];
+
+            $type = isset($mimeTypes[$ext]) && $mimeTypes[$ext];


This will return a boolean and not the mime type, I think this needs to become: $type = $mimeTypes[$ext] ?? ''; or something like that.

Haven't verified the rest of the code, this just popped out to me :)

Thanks for pointing out!

@aredridel apologies I had missed the above. Would you mind taking a look? This might be syntax borrowed from another language -
isset($mimeTypes[$ext])? $mimeTypes[$ext]:''; might be more appropriate. Should anything happen if we encounter a file that is not in the list of file extensions?

fooman · 2017-09-04T02:43:58Z

Hi @aredridel just checking in if there is anything I can help with to get your pull request merged?

fuunnx · 2017-10-03T16:11:45Z

phpserver/router.php

            $debug('file exists');
            return false;
+        } else if (file_exists($file)) {


Sadly it does not take into account other locales than en_US. e.g: the luma theme breaks with 404 errors when the store in in fr_FR.

I did a quick fix on my dev env, testing both fr_FR and en_US static files.

Great PR by the way

fooman · 2017-10-16T06:04:58Z

Apologies @aredridel in not getting this moved forward so far. As Magento had so far zero test coverage in this area I have added a couple of updates to your pull request to add some tests. Please let me know if anything in the latest changes looks out of place to you.

@fuunnx I am keen to hear what additional changes you needed to make to enable a multi-locale environment (I am not sure if this is reliant on a particular setting - for example to add the store code to the url). If you are interested feel free to create a separate pull request once this one has merged.

fuunnx · 2017-10-16T07:21:26Z

@fooman My locale was fr_FR so I hardcoded the following change:
In this code if neither $origFile nor $file exist then check for alternative file by replacing fr_FR in the file path by en_US. Then it works :)

But I don't know the fallback logic magento use internally for this case so I'm not comfortable making a PR =)

aredridel · 2017-10-17T16:40:10Z

Oh nice! I've been intending to come back to this but haven't had time lately!

- skipped the test for bamboo env

- fixed code style

ishakhsuvarov self-assigned this Aug 11, 2017

ishakhsuvarov added this to the August 2017 milestone Aug 11, 2017

ishakhsuvarov added the develop label Aug 11, 2017

Flyingmana reviewed Aug 17, 2017

View reviewed changes

ishakhsuvarov assigned fooman and ishakhsuvarov and unassigned ishakhsuvarov Aug 17, 2017

magento-team added develop Progress: needs update labels Aug 17, 2017

fooman added improvement Infrastructure Progress: accept and removed Progress: needs update labels Aug 18, 2017

ishakhsuvarov removed their assignment Aug 19, 2017

hostep reviewed Aug 19, 2017

View reviewed changes

magento-team added Progress: accept develop improvement Progress: needs update labels Aug 19, 2017

ishakhsuvarov removed the Progress: accept label Aug 19, 2017

magento-engcom-team modified the milestones: August 2017, September 2017 Sep 18, 2017

fuunnx reviewed Oct 3, 2017

View reviewed changes

okorshenko modified the milestones: September 2017, October 2017 Oct 5, 2017

fooman added 3 commits October 16, 2017 18:49

Add a testcase for the phpserver

36231da

Ensure phpserver sends mime type on all existing files

b020517

Remove use_rewrites limitation from phpserver readme

046585f

Update to phpunit 6

0c2df2e

fooman added Progress: accept and removed Progress: needs update labels Oct 16, 2017

magento-team pushed a commit that referenced this pull request Oct 19, 2017

MAGETWO-71529: Improvements to the phpserver router #10500

2a77940

magento-engcom-team changed the base branch from develop to 2.3-develop October 20, 2017 15:32

okorshenko self-assigned this Oct 24, 2017

okorshenko modified the milestones: October 2017, November 2017 Nov 1, 2017

okorshenko merged commit 0c2df2e into magento:2.3-develop Nov 8, 2017

okorshenko pushed a commit that referenced this pull request Nov 8, 2017

MAGETWO-71529: Improvements to the phpserver router #10500

d83ccc4

- skipped the test for bamboo env

okorshenko pushed a commit that referenced this pull request Nov 8, 2017

MAGETWO-71529: Improvements to the phpserver router #10500

3ea60e1

- fixed code style

okorshenko pushed a commit that referenced this pull request Nov 8, 2017

MAGETWO-71529: Improvements to the phpserver router #10500

ada09f8

aredridel deleted the improve-phpserver branch November 10, 2017 03:39

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improvements to the phpserver router #10500

Improvements to the phpserver router #10500

aredridel commented Aug 11, 2017 •

edited by ishakhsuvarov

Loading

magento-cicd2 commented Aug 11, 2017 •

edited

Loading

fooman commented Aug 16, 2017

Flyingmana left a comment

Flyingmana Aug 17, 2017

aredridel Aug 18, 2017

aredridel Aug 18, 2017

Flyingmana Aug 17, 2017

aredridel Aug 18, 2017

aredridel Aug 18, 2017

Flyingmana Aug 17, 2017

aredridel Aug 18, 2017

aredridel Aug 18, 2017

fooman commented Aug 19, 2017

hostep Aug 19, 2017

ishakhsuvarov Aug 19, 2017

fooman Aug 22, 2017

fooman commented Sep 4, 2017

fuunnx Oct 3, 2017

fooman commented Oct 16, 2017 •

edited

Loading

fuunnx commented Oct 16, 2017

aredridel commented Oct 17, 2017

Improvements to the phpserver router #10500

Improvements to the phpserver router #10500

Conversation

aredridel commented Aug 11, 2017 • edited by ishakhsuvarov Loading

magento-cicd2 commented Aug 11, 2017 • edited Loading

fooman commented Aug 16, 2017

Flyingmana left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fooman commented Aug 19, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fooman commented Sep 4, 2017

Choose a reason for hiding this comment

fooman commented Oct 16, 2017 • edited Loading

fuunnx commented Oct 16, 2017

aredridel commented Oct 17, 2017

aredridel commented Aug 11, 2017 •

edited by ishakhsuvarov

Loading

magento-cicd2 commented Aug 11, 2017 •

edited

Loading

fooman commented Oct 16, 2017 •

edited

Loading