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

Fix issue1396: Always read property silently #328

Closed
wants to merge 1 commit into from

Conversation

nikita2206
Copy link
Contributor

No description provided.

@nikita2206 nikita2206 changed the base branch from master to xdebug_2_5 February 7, 2017 19:09
@nikita2206
Copy link
Contributor Author

@derickr here you go

@derickr
Copy link
Contributor

derickr commented Feb 7, 2017

Hi,

thanks for this PR, but it misses a test case. Could you please add one?

You don't have to close this PR, and open another one. Just push another commit to this branch (issue1396) and GitHub will just add it to this PR.

cheers,
Derick

Copy link
Contributor

@derickr derickr left a comment

Choose a reason for hiding this comment

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

Please add a test case.

@nikita2206
Copy link
Contributor Author

Um, I'm afraid there's no infrastructure to test for [presence/absence of] error output. error-output.txt contents are completely ignored by everything

@derickr
Copy link
Contributor

derickr commented Feb 8, 2017

You can just do an echo file_get_contents( sys_get_temp_dir() . /error-output.txt' ); at the bottom of the DBGp test just below https://github.com/xdebug/xdebug/blob/master/tests/bug01165.phpt#L20

@derickr
Copy link
Contributor

derickr commented Mar 4, 2017

Sorry, but I would still like a test case. I have never seen this behaviour in the wild, and hence, I don't feel comfortable adding this patch without a test case.

@derickr
Copy link
Contributor

derickr commented Mar 6, 2017

@nikita2206 — I am still interested in getting this sorted, but if it lingers much longer without a test case I will have to close the PR.

@nikita2206
Copy link
Contributor Author

I've tried doing what you suggested but it didn't really work, maybe because at the time when I was doing file_get_contents() this file still hadn't had anything written to it yet.

I'm away from home, from beginning of February until March 20 and everything is set up at my home PC so I'll be able to make changes only then.

@derickr
Copy link
Contributor

derickr commented Mar 7, 2017

OK, I'll leave it open for a while then! Thanks for checking though. I'm happy to just have a manual test case too. And I'll see what I can do with that.

@derickr
Copy link
Contributor

derickr commented Dec 16, 2017

One more nudge ;-) Would be good to get this into Xdebug 2.6.0.

@derickr
Copy link
Contributor

derickr commented Jan 7, 2018

I finally ran into this myself! It's part of: https://github.com/xdebug/xdebug/pull/399/files#diff-4cce37dc9c7b364c03baa7538ac08798L406

Closing this one out — as it didn't apply any more anyway. Thanks

@derickr derickr closed this Jan 7, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants