Fixing issue #319 #320

emulienfou · 2018-04-26T23:43:03Z

Fixing issue #319.
Based from the PR #185 by @chandon this fix has been ported to Symfony 4 and tested by myself!

(cherry picked from commit 665dc6e)

emulienfou · 2018-06-10T18:22:46Z

@stof what do you think about this PR?

tobias-93 · 2018-06-10T19:00:39Z

As you can see in the tests, you completely change the behavior of the router in some cases. Please make sure you only add new functionality without changing the current behavior. Could you provide some tests with a changed port as well?

emulienfou · 2018-06-21T20:09:48Z

Port extension test added, all tests are up and running!

dmaicher · 2018-07-30T14:14:48Z

~~I just tested this on my app where I use a non-standard port for end-to-end tests and it works nicely~~ 👍

~~Any chance this can be merged? I hope this will not end the same way as #185.~~

emulienfou · 2018-07-30T14:59:36Z

@stof @tobias-93

dmaicher · 2018-07-30T15:21:40Z

@emulienfou actually I made a mistake in my previous tests and it was using a cached response for the route export.

I just tested it again and actually this fix does not work for me. For generated routes it does not correctly append the port for me.

Maybe its because I use multiple hosts and have host restrictions on the routes that I tested?

If I use dev-master from git@github.com:chandon/FOSJsRoutingBundle.git it works.

emulienfou · 2018-07-30T15:29:50Z

Hi @dmaicher, just to be sure, here whats in my fos_js_routes.json file:

{  
   "base_url":"",
   "routes":{
      "projectharmony_message_new":{  
         "tokens":[  
            [  
               "text",
               "\/messages\/new"
            ]
         ],
         "defaults":[  

         ],
         "requirements":[  

         ],
         "hosttokens":[  
            [  
               "text",
               "account.harmonycms.net"
            ]
         ],
         "methods":[  

         ],
         "schemes":[  

         ]
      },
   },
   "prefix":"",
   "host":"localhost:8082",
   "portextension":":8082",
   "scheme":"http"
}

Do you have the portextension key and the port append to the host key?

Do you think port is missing in the hosttokens key?

Actually my modifications is working perfectly in my app, using FOSMessageBundle and others!!!

dmaicher · 2018-07-30T15:46:21Z

@emulienfou

example:

{
  "base_url": "",
  "routes": {
    "some_route_name": {
      "tokens": [
        [
          "variable",
          "\/",
          "[^\/]++",
          "hash"
        ],
        [
          "text",
          "\/ca\/replace"
        ]
      ],
      "defaults": [],
      "requirements": [],
      "hosttokens": [
        [
          "text",
          "my-host.loc"
        ]
      ],
      "methods": [
        "GET",
        "POST"
      ],
      "schemes": []
    },
  },
  "prefix": "",
  "host": "my-host.loc:81",
  "portextension": ":81",
  "scheme": "http"
}

So now I am on http://my-host.loc:81/... and I generate the route using

Routing.generate('some_route_name', {'hash':'foo'});

Now gives me: http://my-host.loc/ca/replace/foo

With the PR #185 I get /ca/replace/foo which is relative and works with the correct port then.

emulienfou · 2018-07-30T16:10:08Z

So weird, I got the same fix but not the same output.

In your composer.lock you are using the commit 42fd6a7234aea6782a94f5eb5328940006347150 on my fork repo?

Because I get the url, by example http://marketplace.harmonycms.net::8082/vote/1/theme when generating the route like you do!

dmaicher · 2018-07-30T17:12:13Z

@emulienfou yes I'm using that commit.

But the two fixes are clearly different here:

https://github.com/FriendsOfSymfony/FOSJsRoutingBundle/pull/320/files#diff-5163afca7bc7f651c818adbc667b9877R351

https://github.com/FriendsOfSymfony/FOSJsRoutingBundle/pull/185/files#diff-8e1e018bfe2bd15df10d1864a922ff0cR256

😉 That's the issue for me.

Append color `:` only when *host + port* are used in JS and Extractor

emulienfou · 2018-08-01T21:30:03Z

@tobias-93 Issue fixed
@dmaicher can you do a test with your app ?

FYI: Travis fail only on PHP5.3, due to travis, not my PR, check logs

dmaicher

works now 👍

small comments on how to simplify it

dmaicher · 2018-08-02T18:47:49Z

Extractor/ExposedRoutesExtractor.php

@@ -104,14 +104,26 @@ public function getHost()
    {
        $requestContext = $this->router->getContext();

-        $host = $requestContext->getHost();
+        $host = $requestContext->getHost() .
+            ('' === $this->getPortExtension() ? $this->getPortExtension() : ':' . $this->getPortExtension());


the colon should be added here instead I believe

https://github.com/FriendsOfSymfony/FOSJsRoutingBundle/pull/320/files#diff-0993cf33f09a40107008a6597f40c4f6R123

Like it was done here also:

https://github.com/FriendsOfSymfony/FOSJsRoutingBundle/pull/185/files#diff-0993cf33f09a40107008a6597f40c4f6R122

Otherwise this comment is wrong: https://github.com/FriendsOfSymfony/FOSJsRoutingBundle/pull/320/files#diff-1955b791bdfe6c6e2f87f9dd89d24413R53

I remove the colon : because the tests are wrong.

For me, portextension should only contain port number without colon.

In the JS test case , we need to test with portextension: ':443' instead of portextension: 443

And yes, I think the comment is wrong and need to be changed

Why is it called portExtension then? can be just port?

Yes, good question.
This came from the PR #185

So, I can rename portExtension by port. I think, like you said, everyone will know what it is ?

dmaicher · 2018-08-02T18:48:15Z

Resources/js/router.js

@@ -263,8 +281,8 @@ class Router {
            url = route.requirements["_scheme"] + "://" + (host || this.getHost()) + url;
        } else if ("undefined" !== typeof route.schemes && "undefined" !== typeof route.schemes[0] && this.getScheme() !== route.schemes[0]) {
            url = route.schemes[0] + "://" + (host || this.getHost()) + url;
-        } else if (host && this.getHost() !== host) {
-            url = this.getScheme() + "://" + host + url;
+        } else if (host && this.getHost() !== host + ('' === portextension ? '' : ':' + portextension)) {


this can then just be like here:

https://github.com/FriendsOfSymfony/FOSJsRoutingBundle/pull/185/files#diff-8e1e018bfe2bd15df10d1864a922ff0cR257

This part of code has been changed, because I remove the colon : in the portextension variable.
Like I said before, the portextension variable should only contain an int value with the port number

lsmith77 · 2018-08-07T11:02:47Z

can you check if you can make the tests pass on PHP 5.6?
otherwise we might just go to PHP 7 as the minimum requirement.

emulienfou · 2018-08-07T16:59:47Z

@lsmith77 did we deprecate PHP 5.3 too from composer.json file?

rvmourik · 2018-09-29T10:10:01Z

Any progress on getting this merged?

lsmith77 · 2018-09-29T11:05:05Z

not using the Bundle but took the “risk” of merging it since it looks good and the tests pass.

rvmourik · 2018-09-29T11:06:38Z

@lsmith77 Thanks, are you going to make a release so i can update my composer without pinning it to the dev-master?

lsmith77 · 2018-09-29T11:07:53Z

https://github.com/FriendsOfSymfony/FOSJsRoutingBundle/releases/tag/2.2.1

chandon and others added 3 commits April 26, 2018 18:43

Add HTTP non standard port support for route with specific host

21eafab

(cherry picked from commit 665dc6e)

Gulp sources

13708a0

Fixing JS, missing call to setPortExtension()

f61c519

emulienfou mentioned this pull request Apr 26, 2018

[Still unfixed] Non standard used port #319

Closed

Fixing test failure

254f896

emulienfou and others added 5 commits June 11, 2018 08:52

Merge branch 'master' into master

5972e15

Update code to undefined portextension

80ff420

Fixing code, adding missing colon separator

793bcdd

Adding portextension tests

9677c2f

Update sources and gulped version

42fd6a7

David Sanchez added 3 commits July 30, 2018 13:41

Fixing issue reported by @dmaicher in: #320 (comment)

5e24c18

Fixing issue about portextension.

59acf34

Append color `:` only when *host + port* are used in JS and Extractor

Fixing condition

1d1becb

dmaicher reviewed Aug 2, 2018

View reviewed changes

Renaming portExtension to port

18a9219

Remove deprecated PHP 5.3 to 5.6

2ccdbb5

lsmith77 merged commit fce90c6 into FriendsOfSymfony:master Sep 29, 2018

rakelley mentioned this pull request Sep 24, 2019

All absolute url generation should include ports #361

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixing issue #319 #320

Fixing issue #319 #320

emulienfou commented Apr 26, 2018

emulienfou commented Jun 10, 2018

tobias-93 commented Jun 10, 2018

emulienfou commented Jun 21, 2018

dmaicher commented Jul 30, 2018 •

edited

Loading

emulienfou commented Jul 30, 2018

dmaicher commented Jul 30, 2018 •

edited

Loading

emulienfou commented Jul 30, 2018

dmaicher commented Jul 30, 2018

emulienfou commented Jul 30, 2018

dmaicher commented Jul 30, 2018

emulienfou commented Aug 1, 2018

dmaicher left a comment

dmaicher Aug 2, 2018

emulienfou Aug 2, 2018 •

edited

Loading

dmaicher Aug 2, 2018

emulienfou Aug 2, 2018

emulienfou Aug 2, 2018 •

edited

Loading

dmaicher Aug 2, 2018

emulienfou Aug 2, 2018

lsmith77 commented Aug 7, 2018

emulienfou commented Aug 7, 2018

rvmourik commented Sep 29, 2018

lsmith77 commented Sep 29, 2018

rvmourik commented Sep 29, 2018

lsmith77 commented Sep 29, 2018

Fixing issue #319 #320

Fixing issue #319 #320

Conversation

emulienfou commented Apr 26, 2018

emulienfou commented Jun 10, 2018

tobias-93 commented Jun 10, 2018

emulienfou commented Jun 21, 2018

dmaicher commented Jul 30, 2018 • edited Loading

emulienfou commented Jul 30, 2018

dmaicher commented Jul 30, 2018 • edited Loading

emulienfou commented Jul 30, 2018

dmaicher commented Jul 30, 2018

emulienfou commented Jul 30, 2018

dmaicher commented Jul 30, 2018

emulienfou commented Aug 1, 2018

dmaicher left a comment

Choose a reason for hiding this comment

dmaicher Aug 2, 2018

Choose a reason for hiding this comment

emulienfou Aug 2, 2018 • edited Loading

Choose a reason for hiding this comment

dmaicher Aug 2, 2018

Choose a reason for hiding this comment

emulienfou Aug 2, 2018

Choose a reason for hiding this comment

emulienfou Aug 2, 2018 • edited Loading

Choose a reason for hiding this comment

dmaicher Aug 2, 2018

Choose a reason for hiding this comment

emulienfou Aug 2, 2018

Choose a reason for hiding this comment

lsmith77 commented Aug 7, 2018

emulienfou commented Aug 7, 2018

rvmourik commented Sep 29, 2018

lsmith77 commented Sep 29, 2018

rvmourik commented Sep 29, 2018

lsmith77 commented Sep 29, 2018

dmaicher commented Jul 30, 2018 •

edited

Loading

dmaicher commented Jul 30, 2018 •

edited

Loading

emulienfou Aug 2, 2018 •

edited

Loading

emulienfou Aug 2, 2018 •

edited

Loading