Skip to content

Lost commits during merge? #24487

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

Closed
webmake opened this issue Oct 8, 2017 · 4 comments
Closed

Lost commits during merge? #24487

webmake opened this issue Oct 8, 2017 · 4 comments

Comments

@webmake
Copy link

webmake commented Oct 8, 2017

Q A
Bug report? yes
Symfony version 3.2.8-3.3.0

I've just updated symfony/http-foundation dependency from 3.2.8 to 3.3.10, and tests at random place just broke, previously was working with expectation of 'http://localhost/test', now it became 'http://localhosttest'. I'm using getUri method.

I made some research, so I updated to 3.2.13 (seems working version). then I updated to 3.3.0 version (broke), and tried to find in history who deleted '/', but nothing interested found, just addition, but not removal, at commit symfony/http-foundation@9077baf#diff-7edb274bc39ed8c493badba7dd278826

+        if ($requestUri !== '' && $requestUri[0] !== '/') {
+            $requestUri = '/'.$requestUri;
+        }

I'm using getUri method, which uses that discussed preparePathInfo method. More over I found another missing slashes commits:
symfony/http-foundation@9077baf#diff-7edb274bc39ed8c493badba7dd278826
symfony/http-foundation@4665cb9#diff-7edb274bc39ed8c493badba7dd278826
symfony/http-foundation@742db41#diff-7edb274bc39ed8c493badba7dd278826L1849

Please clearify is it missing or deleted for real but no reference to it?

@webmake
Copy link
Author

webmake commented Oct 8, 2017

image

According to my diff tool f594c12 missing, 9077baf contains, 4665cb9 contains,
742db41 contains,
8aaa59f contains (initial commit)

@nicolas-grekas
Copy link
Member

Might be a bad merge conflict resolution. Would you mind sending a fix with a test case please?

@webmake
Copy link
Author

webmake commented Oct 8, 2017

Test case for request:

public function testRequestMatches()
    {
        $inputArray = [
            'input1' => 'value1',
            'input2' => 'value2',
        ];

        $request = Request::create('test', 'POST', $inputArray);
        $expect = [
            'request_uri' => 'http://localhost/test',
            'request_method' => 'POST',
            'request_input' => $inputArray,
            'request_headers' => $request->headers,
        ];
        $actual = [
            'request_uri' => $request->getUri(),
            'request_method' => $request->getMethod(),
            'request_input' => $request->request->all(),
            'request_headers' => $request->headers,
        ];

        $this->assertEquals($expect, $actual);
    }
    protected function preparePathInfo()
    {
        $baseUrl = $this->getBaseUrl();

        if (null === ($requestUri = $this->getRequestUri())) {
            return '/';
        }

        // Remove the query string from REQUEST_URI
        if (false !== $pos = strpos($requestUri, '?')) {
            $requestUri = substr($requestUri, 0, $pos);
        }
        if ($requestUri !== '' && $requestUri[0] !== '/') {
            $requestUri = '/'.$requestUri;
        }

        $pathInfo = substr($requestUri, strlen($baseUrl));
        if (null !== $baseUrl && (false === $pathInfo || '' === $pathInfo)) {
            // If substr() returns false then PATH_INFO is set to an empty string
            return '/';
        } elseif (null === $baseUrl) {
            return $requestUri;
        }

        return (string) $pathInfo;
    }

Not sure only due prepareBaseUrl, because according commit it is necessary to add

        if ($requestUri !== '' && $requestUri[0] !== '/') {
            $requestUri = '/'.$requestUri;
        }

but seems that works without it also

@Simperfit
Copy link
Contributor

Could you please send a pull request with the following changes ?

@fabpot fabpot closed this as completed Oct 16, 2017
fabpot added a commit that referenced this issue Oct 16, 2017
…h a question mark. (syzygymsu)

This PR was merged into the 3.3 branch.

Discussion
----------

[3.3] Fixed pathinfo calculation for requests starting with a question mark.

| Q             | A
| ------------- | ---
| Branch?       | 3.3
| Bug fix?      | yes
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | #24487
| License       | MIT
| Doc PR        | no

Fix of bad merge conflict resolving as mentioned in #24487. Port #21968 to 3.3+

Commits
-------

c17a922 Fixed pathinfo calculation for requests starting with a question mark.  - fix bad conflict resolving issue  - port #21968 to 3.3+
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

No branches or pull requests

4 participants