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 #519 Prevent infinite loop when getting references from invalid range #521

Conversation

rdarcy1
Copy link
Contributor

@rdarcy1 rdarcy1 commented May 30, 2018

Fixes #519

Didn't realise it was such an easy fix..! Currently it returns an empty array of references when passed an invalid range, not sure if an exception would be more suitable.

rdarcy1 added 2 commits May 30, 2018 22:54

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
Copy link
Member

@PowerKiKi PowerKiKi left a comment

Choose a reason for hiding this comment

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

I think it would be best to throw an exception here, because an invalid range means something is wrong and should be fixed "before" this method. Also could you please cover this with unit tests ?

@rdarcy1
Copy link
Contributor Author

rdarcy1 commented Jun 1, 2018

I've updated to throw an exception on invalid range and amended the unit test to match.

@criztovyl
Copy link
Contributor

Try executing ./vendor/bin/php-cs-fixer fix to fix the remaining code-style issues that Travis does not like.

Regarding Scrutinizer I am not sure, but last time PowerKiki suggested to extract the added code into it's own method, maybe try that. :)

Thx!

@criztovyl
Copy link
Contributor

In this case just your added code in extractAllCellReferencesInRange() would need to be extracted.

@rdarcy1
Copy link
Contributor Author

rdarcy1 commented Jun 2, 2018

I've run cs fixer but it's not changing anything, am I doing something wrong?

This is the output:

vendor/bin/php-cs-fixer fix
Loaded config default from "/Users/robin/Code/PhpSpreadsheet/.php_cs.dist".

Fixed all files in 51.650 seconds, 30.000 MB memory used

But no files have been changed.

I'll refactor the invalid range check – the whole class isn't too tidy, my code must have just tipped Scrutinizer over the fail threshold.

@rdarcy1
Copy link
Contributor Author

rdarcy1 commented Jun 4, 2018

I've also refactored the large extractAllCellReferencesInRange method down to a couple of smaller methods and extracted a couple of duplicated lines to improve the code rating – hopefully that's not outside the scope of the original PR?

I think much of the logic concerning cell ranges and references could be extracted to their own objects rather than dealing with range and reference arrays if you were open to cleaning up the class further.

@criztovyl
Copy link
Contributor

By personal preference I would have only extracted as much as is required to pass CI and included everything else in a additional PR. :)
But that seems to be exactly what you did ^^

Could provide an additional PR for your suggested refactorings? Otherwise you could open an a feature request issue.

Thanks for your contributions so far. :)

@PowerKiKi PowerKiKi closed this in ed21854 Jun 10, 2018
@PowerKiKi
Copy link
Member

Thanks ! The refactoring is always welcomed. The existing code tends to use way too big methods...

@boredom2
Copy link

Hi there,
we currently experience Tons of Problems with this Change :)
After a quick Investigation, I suppose, that $startCol>=$endCol wrongly fails in the case "B" >= "AG".
Obviously, "B" is smaller than "AG", but due to PHPs String Comparison, it returns false and therefore wrongly emits the Exception.

@rdarcy1
Copy link
Contributor Author

rdarcy1 commented Jun 11, 2018

Ah, sorry I wasn't thorough enough with my tests then. I can re-submit with a better comparison function.

@PowerKiKi
Copy link
Member

@boredom2, I can confirm what you say. It was merged too quickly. It was also reported in #545, and I just pushed a fix on develop branch. Can you confirm it fixes the issue for you ?

@PowerKiKi
Copy link
Member

It's been released as 1.3.1

billblume pushed a commit to billblume/PhpSpreadsheet that referenced this pull request Jun 13, 2018
Dfred pushed a commit to Dfred/PhpSpreadsheet that referenced this pull request Nov 20, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

None yet

4 participants