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

Avoid access violation #732

Merged
merged 5 commits into from
Sep 12, 2018
Merged

Avoid access violation #732

merged 5 commits into from
Sep 12, 2018

Conversation

GlazerMann
Copy link
Collaborator

@GlazerMann GlazerMann commented Sep 6, 2018

The current code crashes

@codecov
Copy link

codecov bot commented Sep 6, 2018

Codecov Report

Merging #732 into master will decrease coverage by 0.84%.
The diff coverage is 50%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master     #732      +/-   ##
============================================
- Coverage      69.3%   68.45%   -0.85%     
- Complexity     1621     1637      +16     
============================================
  Files            12       12              
  Lines          3264     3313      +49     
============================================
+ Hits           2262     2268       +6     
- Misses         1002     1045      +43
Impacted Files Coverage Δ Complexity Δ
Template.php 73.79% <50%> (-2.29%) 1457 <10> (+7)
apiFunctions.php 70.21% <50%> (+2.71%) 0 <0> (ø) ⬇️
Comment.php 100% <0%> (ø) 12% <0%> (+8%) ⬆️
expandFns.php 90.71% <0%> (+0.71%) 0% <0%> (ø) ⬇️
Page.php 74.64% <0%> (+1.28%) 71% <0%> (+1%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ee53e0e...147bdc1. Read the comment docs.

@GlazerMann GlazerMann changed the title Avoid using protected methods Avoid access violation Sep 8, 2018
Copy link
Owner

@ms609 ms609 left a comment

Choose a reason for hiding this comment

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

Just to check: Does the unset() FORCE a forget of the URL, which might be overriden / cancelled by checks in forget?

@GlazerMann
Copy link
Collaborator Author

It does force it, but it does not unset via= etc. I don’t think forget has any checks. Also the unset code does crash since all the param stuff is private/protected

Copy link
Owner

@ms609 ms609 left a comment

Choose a reason for hiding this comment

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

Ah, the reason that I didn't use forget here is given in the comment on the previous line; is it still desirable to supress the echo? If not, let's remove the comment; if so, let's add a new function quietly_forget.

@GlazerMann
Copy link
Collaborator Author

@ms609 done

@ms609 ms609 merged commit d22aac9 into ms609:master Sep 12, 2018
@GlazerMann GlazerMann deleted the patch-9 branch September 12, 2018 16:22
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

Successfully merging this pull request may close these issues.

None yet

2 participants