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

Construct path to the version file from the current directory and fil… #24673

Merged
merged 1 commit into from Jun 3, 2016

Conversation

VicDeo
Copy link
Member

@VicDeo VicDeo commented May 17, 2016

…ename. Fixes #22450

@VicDeo VicDeo added this to the 9.1-current milestone May 17, 2016
@mention-bot
Copy link

By analyzing the blame information on this pull request, we identified @schiesbn, @bartv2 and @icewind1991 to be potential reviewers

} else {
$versionsBegin = strrpos($file['path'], '.v');
$versionsBegin = strrpos($$filePath, '.v');
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

two dollars? looks suspicious

Mind adjusting the unit test as well to properly cover this, if possible ?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@PVince81 sorry, it slipped through. Surely only one.
I don't know how to reproduce it.
The only thing I know that it works and that getDirectoryContents produces path relative to datadir instead if view root under some unclear conditions.

@PVince81
Copy link
Contributor

Weird, so this means we can't trust the output of getDirectoryContents ?
There might be other places where shit can go wrong because of that 😦

Would be good to find out what the root cause is.

@PVince81
Copy link
Contributor

@VicDeo can you add a unit test ?

@icewind1991
Copy link
Contributor

You can't trust the output of $fileInfo['path']

$fileInfo->getPath() can be trusted though.

The difference between the two is due to legacy reasons and I'm not sure if I can fix ['path'] without breaking something

@PVince81
Copy link
Contributor

@icewind1991 @VicDeo so should we merge this without unit tests ??

@VicDeo
Copy link
Member Author

VicDeo commented May 31, 2016

@PVince81 getAllVersions is private and static so I'd say yes.

@PVince81
Copy link
Contributor

PVince81 commented Jun 1, 2016

Ok then...

@icewind1991 can you confirm that this PR is safe ?

@icewind1991
Copy link
Contributor

👍 looks good

@PVince81
Copy link
Contributor

PVince81 commented Jun 2, 2016

Ok, thanks guys. I rebased one last time for CI.

👍

@PVince81 PVince81 merged commit f206423 into master Jun 3, 2016
@PVince81 PVince81 deleted the construct-path branch June 3, 2016 08:50
@VicDeo
Copy link
Member Author

VicDeo commented Jun 7, 2016

@cmonteroluque initailly reported for 8.2. Backport?

@PVince81
Copy link
Contributor

PVince81 commented Jun 7, 2016

@VicDeo okay please backport down to 8.2

CC @dragotin @DeepDiver1975

@VicDeo
Copy link
Member Author

VicDeo commented Jun 8, 2016

Stable9: #25039
Stable8.2: #25040

@lock
Copy link

lock bot commented Aug 5, 2019

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@lock lock bot locked as resolved and limited conversation to collaborators Aug 5, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[8.2.2] Cron job creates error
4 participants