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

Make sure that files that are siblings of directories, are reported as files #982

Conversation

@nickvergessen
Copy link
Contributor

@nickvergessen nickvergessen commented Jul 13, 2017

Without the patch, the two files are reported to be a collection:

...
	<d:response>
		<d:href>/col/col/test.txt</d:href>
		<d:propstat>
			<d:prop>
				<d:resourcetype>
					<d:collection/>
				</d:resourcetype>
			</d:prop>
			<d:status>HTTP/1.1 200 OK</d:status>
		</d:propstat>
	</d:response>
...
	<d:response>
		<d:href>/dir/child.txt</d:href>
		<d:propstat>
			<d:prop>
				<d:resourcetype>
					<d:collection/>
				</d:resourcetype>
			</d:prop>
			<d:status>HTTP/1.1 200 OK</d:status>
		</d:propstat>
	</d:response>
...

Reported in nextcloud/server#5543 Proposed patch is from @blancjerome I just wrote the test case and cleaned it up a bit...

@nickvergessen nickvergessen changed the title Make sure that files that are children of directories, are reported as files Make sure that files that are siblings of directories, are reported as files Jul 13, 2017
@PVince81
Copy link
Contributor

@PVince81 PVince81 commented Oct 9, 2017

Travis failures, not sure if related:

1) Sabre\HTTP\SapiTest::testSendLimitedByContentLengthStream

TypeError: stream_copy_to_stream() expects parameter 3 to be integer, string given

/home/travis/build/fruux/sabre-dav/vendor/sabre/http/lib/Sapi.php:93

/home/travis/build/fruux/sabre-dav/vendor/sabre/http/tests/HTTP/SapiTest.php:158
@DeepDiver1975
Copy link
Member

@DeepDiver1975 DeepDiver1975 commented Oct 9, 2017

Who has to power to merge? @evert (sorry to wake you up) @staabm @DominikTo ?

@staabm
Copy link
Member

@staabm staabm commented Oct 9, 2017

I cannnot merge it as long as the build isnt green.

Just restarted the build.

I cannot say whether this fix is correct. Will take your word for it.

@DominikTo
Copy link
Member

@DominikTo DominikTo commented Oct 9, 2017

Changelog needs update, too.

@DeepDiver1975
Copy link
Member

@DeepDiver1975 DeepDiver1975 commented Oct 9, 2017

Changelog needs update, too.

@nickvergessen can you please take care? THX

@nickvergessen
Copy link
Contributor Author

@nickvergessen nickvergessen commented Oct 10, 2017

I just saw that you do forward merges? So should I send my PR against 3.2 and add it to the changelog in the 3.2 block?

@DeepDiver1975
Copy link
Member

@DeepDiver1975 DeepDiver1975 commented Oct 10, 2017

I just saw that you do forward merges? So should I send my PR against 3.2 and add it to the changelog in the 3.2 block?

@DominikTo @staabm you are deeper involved here - what is the correct workflow? THX
(Or is that documented and I did not read properly? 😉 )

@staabm
Copy link
Member

@staabm staabm commented Oct 10, 2017

in case this is considered a bugfix you should merge/cherry-pick it into the lowest/oldest branch which is applicable for it (e.g. oldest yet maintained one) and merge into newer ones.

IIRC if your git fu is great enough you can do this via commandline, no matter what/where the Pull Request is pointing to. its easier for the maintainer when the base branch of this PR is already pointing to the right branch.

…s files

Signed-off-by: Joas Schilling <coding@schilljs.com>
@nickvergessen nickvergessen force-pushed the nickvergessen:fix-invalid-propfind-data-on-infinit-requests branch from 020fa28 to 0cef604 Oct 10, 2017
@nickvergessen nickvergessen changed the base branch from master to 3.2 Oct 10, 2017
@nickvergessen
Copy link
Contributor Author

@nickvergessen nickvergessen commented Oct 10, 2017

Rebased onto 3.2 (version we are using) since I didn't find info what is still supported

3.1 is EOL since June 2017: http://sabre.io/dav/install/

@staabm
staabm approved these changes Oct 10, 2017
@nickvergessen
Copy link
Contributor Author

@nickvergessen nickvergessen commented Oct 10, 2017

3.1 is EOL since June 2017: http://sabre.io/dav/install/

So it seems to be correct

@DeepDiver1975 DeepDiver1975 merged commit 0a21668 into sabre-io:3.2 Oct 10, 2017
@DeepDiver1975
Copy link
Member

@DeepDiver1975 DeepDiver1975 commented Oct 10, 2017

and now cherry-pick to master and 3.3? with/without pull request?

@blancjerome
Copy link

@blancjerome blancjerome commented Oct 10, 2017

@nickvergessen
Copy link
Contributor Author

@nickvergessen nickvergessen commented Oct 10, 2017

They seem to just merge it forward, so

  • git checkout 3.3
  • git merge --no-ff 3.2
  • git checkout master
  • git merge --no-ff 3.3
  • git push origin 3.3 master
@DeepDiver1975
Copy link
Member

@DeepDiver1975 DeepDiver1975 commented Oct 10, 2017

Done

@nickvergessen nickvergessen deleted the nickvergessen:fix-invalid-propfind-data-on-infinit-requests branch Oct 10, 2017
@nickvergessen
Copy link
Contributor Author

@nickvergessen nickvergessen commented Oct 10, 2017

I think you merged 3.2 in master instead of 3.3 ;)

anyway thanks for getting this forward :)

@evert
Copy link
Member

@evert evert commented Oct 10, 2017

Just fyi, the 3.3 branch was intended to just displace the 3.2. It's been backward compatible, but it has a few small improvements. So if you were to drop 3.2 and do a release of the 3.3 branch, things should be fine.

@LorbusChris
Copy link

@LorbusChris LorbusChris commented Feb 7, 2018

Can we get a new release with this PR?

@staabm
Copy link
Member

@staabm staabm commented Feb 8, 2018

for a new release we might also update the bundled sabre/xml

https://github.com/sabre-io/xml/releases/tag/2.1.0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

9 participants