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

Reader::createFromPath() opens the file in write mode #258

Closed
Bilge opened this Issue Sep 11, 2017 · 21 comments

Comments

Projects
None yet
4 participants
@Bilge

Bilge commented Sep 11, 2017

Issue summary

Although I realise createFromPath() is not implemented directly by Reader, the fact that this calling convention exists and yet does not open the file in read-only mode is a terrible design flaw because it's misleading and error-prone. If this method must be implemented in an abstract class, and if it must default to requiring write mode access, Reader must override the default in order for its behaviour to be sane and predictable.

Expected behaviour

Method File access mode
Reader::createFromPath() read
Writer::createFromPath() write

Actual behaviour

Method File access mode
Reader::createFromPath() read, write
Writer::createFromPath() write

No justification

As shown above, the Reader demands write access despite the fact that it should not need it because it's a reader. Just because this fact is poorly documented does not imply justification; there is no justification for this bizarre design decision in the documentation. Indeed, unless one is intimately familiar with PHP file open nomenclature, simply stating that the default is r+ does not clearly convey that even the Reader class will demand write modes.

As an aside, the default should probably include the b flag to avoid unexpected data translation.


Continued from #256. Please do not close this issue prematurely.

@nyamsprod

This comment has been minimized.

Show comment
Hide comment
@nyamsprod

nyamsprod Sep 11, 2017

Member

#256 was closed because of no real issue and poor remarks from your part. If you found the documentation lacking please provide a PR to improve them or at least be constructive. Same with this issue. Instead of complaining please provide a PR as I found no issue with the current API which is documented. I don't tolerate poor remarks or attitude on open source projects that I maintain. If your don't like the current API you are still free to fork the project and do as you please but orherwise I would request that you calm down.

Member

nyamsprod commented Sep 11, 2017

#256 was closed because of no real issue and poor remarks from your part. If you found the documentation lacking please provide a PR to improve them or at least be constructive. Same with this issue. Instead of complaining please provide a PR as I found no issue with the current API which is documented. I don't tolerate poor remarks or attitude on open source projects that I maintain. If your don't like the current API you are still free to fork the project and do as you please but orherwise I would request that you calm down.

@Bilge

This comment has been minimized.

Show comment
Hide comment
@Bilge

Bilge Sep 11, 2017

@nyamsprod You are completely missing the point and taking this too personally. This has nothing to do with documentation; it is a design flaw with the defaults you have chosen for file access modes for the Reader class. The default file access mode should not include the write attribute. This cannot be fixed with documentation, it can only be fixed in code.

Bilge commented Sep 11, 2017

@nyamsprod You are completely missing the point and taking this too personally. This has nothing to do with documentation; it is a design flaw with the defaults you have chosen for file access modes for the Reader class. The default file access mode should not include the write attribute. This cannot be fixed with documentation, it can only be fixed in code.

@nyamsprod

This comment has been minimized.

Show comment
Hide comment
@nyamsprod

nyamsprod Sep 11, 2017

Member

@Bilge submit a PR it will be reviewed and if it is found useful it will be merged. Thanks

Member

nyamsprod commented Sep 11, 2017

@Bilge submit a PR it will be reviewed and if it is found useful it will be merged. Thanks

@Bilge

This comment has been minimized.

Show comment
Hide comment
@Bilge

Bilge Sep 11, 2017

Bilge commented Sep 11, 2017

@nyamsprod

This comment has been minimized.

Show comment
Hide comment
@nyamsprod

nyamsprod Sep 11, 2017

Member

Yes but it will have to wait until the next major release

Member

nyamsprod commented Sep 11, 2017

Yes but it will have to wait until the next major release

@Bilge

This comment has been minimized.

Show comment
Hide comment
@Bilge

Bilge Sep 11, 2017

Bilge commented Sep 11, 2017

@nyamsprod nyamsprod self-assigned this Sep 12, 2017

@nyamsprod

This comment has been minimized.

Show comment
Hide comment
@nyamsprod

nyamsprod Sep 12, 2017

Member

Since :

  • v9 has been recently released
  • there's more or less 18 months between v8 and v9
  • and it would require real major improvement (which is the case between both version)

Currently I can not anwser this question.

Member

nyamsprod commented Sep 12, 2017

Since :

  • v9 has been recently released
  • there's more or less 18 months between v8 and v9
  • and it would require real major improvement (which is the case between both version)

Currently I can not anwser this question.

@Bilge

This comment has been minimized.

Show comment
Hide comment
@Bilge

Bilge Sep 12, 2017

I don't find any of those points compelling. What is the problem with releasing a new major version tomorrow? Nobody is forced to upgrade; they do so at their convenience. However, if you're committing to not releasing the fix for 18 months I will not write one since you are actively discouraging me from doing so.

Bilge commented Sep 12, 2017

I don't find any of those points compelling. What is the problem with releasing a new major version tomorrow? Nobody is forced to upgrade; they do so at their convenience. However, if you're committing to not releasing the fix for 18 months I will not write one since you are actively discouraging me from doing so.

@nyamsprod

This comment has been minimized.

Show comment
Hide comment
@nyamsprod

nyamsprod Sep 12, 2017

Member

I understand your frustration but I too don't find the need to release a major release just to fix something which is IMHO not broken.

Member

nyamsprod commented Sep 12, 2017

I understand your frustration but I too don't find the need to release a major release just to fix something which is IMHO not broken.

@Bilge

This comment has been minimized.

Show comment
Hide comment
@Bilge

Bilge Sep 12, 2017

I cannot understand how you can possibly think it's not broken. Allow me to demonstrate with a proof of concept.

  1. touch foo.csv
  2. chmod a-w foo.csv
  3. namei -m foo.csv

-r--r--r-- foo.csv

  1. (League\Csv\Reader::createFromPath('foo.csv'))->fetchOne();

PHP Fatal error: Uncaught RuntimeException: SplFileObject::__construct(foo.csv): failed to open stream: Permission denied in /srv/wa-api/vendor/league/csv/src/AbstractCsv.php:271
Stack trace:
#0 /srv/wa-api/vendor/league/csv/src/AbstractCsv.php(271): SplFileObject->__construct('foo.csv', 'r+')
#1 /srv/wa-api/vendor/league/csv/src/AbstractCsv.php(253): League\Csv\AbstractCsv->setIterator()
#2 /srv/wa-api/vendor/league/csv/src/Modifier/QueryFilter.php(160): League\Csv\AbstractCsv->getIterator()
#3 /srv/wa-api/vendor/league/csv/src/Reader.php(119): League\Csv\AbstractCsv->getQueryIterator()
#4 {main}
thrown in /srv/wa-api/vendor/league/csv/src/AbstractCsv.php on line 271

Bilge commented Sep 12, 2017

I cannot understand how you can possibly think it's not broken. Allow me to demonstrate with a proof of concept.

  1. touch foo.csv
  2. chmod a-w foo.csv
  3. namei -m foo.csv

-r--r--r-- foo.csv

  1. (League\Csv\Reader::createFromPath('foo.csv'))->fetchOne();

PHP Fatal error: Uncaught RuntimeException: SplFileObject::__construct(foo.csv): failed to open stream: Permission denied in /srv/wa-api/vendor/league/csv/src/AbstractCsv.php:271
Stack trace:
#0 /srv/wa-api/vendor/league/csv/src/AbstractCsv.php(271): SplFileObject->__construct('foo.csv', 'r+')
#1 /srv/wa-api/vendor/league/csv/src/AbstractCsv.php(253): League\Csv\AbstractCsv->setIterator()
#2 /srv/wa-api/vendor/league/csv/src/Modifier/QueryFilter.php(160): League\Csv\AbstractCsv->getIterator()
#3 /srv/wa-api/vendor/league/csv/src/Reader.php(119): League\Csv\AbstractCsv->getQueryIterator()
#4 {main}
thrown in /srv/wa-api/vendor/league/csv/src/AbstractCsv.php on line 271

@nyamsprod

This comment has been minimized.

Show comment
Hide comment
@nyamsprod

nyamsprod Sep 12, 2017

Member

After reading the documentation it becomes

(League\Csv\Reader::createFromPath('foo.csv', 'r'))->fetchOne();

Et voilà !

Member

nyamsprod commented Sep 12, 2017

After reading the documentation it becomes

(League\Csv\Reader::createFromPath('foo.csv', 'r'))->fetchOne();

Et voilà !

@Bilge

This comment has been minimized.

Show comment
Hide comment
@Bilge

Bilge Sep 12, 2017

You are completely missing the point. The default behaviour for a class called Reader must not require write permissions in order to work. This betrays user expectations and is therefore wrong. Just because you have documented facility to override the default (incorrect) behaviour does not make this acceptable.

Bilge commented Sep 12, 2017

You are completely missing the point. The default behaviour for a class called Reader must not require write permissions in order to work. This betrays user expectations and is therefore wrong. Just because you have documented facility to override the default (incorrect) behaviour does not make this acceptable.

@nyamsprod

This comment has been minimized.

Show comment
Hide comment
@nyamsprod

nyamsprod Sep 12, 2017

Member

Can we agree to disagree. I understand the changes you want to make but I don't think releasing a new major release just because someone refuse to read the doc is compelling argument either. Yes a new version will improve its settings thanks to your remarks but in the meantime if one read the doc, the solution is crystal clear.

Again, if you don't want to wait for a new major release you have plenty of solutions by using SOLID principals or by forking the package.

Member

nyamsprod commented Sep 12, 2017

Can we agree to disagree. I understand the changes you want to make but I don't think releasing a new major release just because someone refuse to read the doc is compelling argument either. Yes a new version will improve its settings thanks to your remarks but in the meantime if one read the doc, the solution is crystal clear.

Again, if you don't want to wait for a new major release you have plenty of solutions by using SOLID principals or by forking the package.

@Bilge

This comment has been minimized.

Show comment
Hide comment
@Bilge

Bilge Sep 12, 2017

You must think I'm absolutely stupid; I'm not waiting for you to release a new version, we'd already pushed a "fix" by overriding the default before this ticket was ever created. This ticket was created solely as a gift to you and your project, to show you a flaw that you could fix to improve the library's quality, as recompense for your efforts putting it together. But you refuse to see any flaws, instead pretending that documented workarounds - not that they are even presented as such - somehow justify incorrect behaviour. You will be the death of your own project with an attitude like this.

Bilge commented Sep 12, 2017

You must think I'm absolutely stupid; I'm not waiting for you to release a new version, we'd already pushed a "fix" by overriding the default before this ticket was ever created. This ticket was created solely as a gift to you and your project, to show you a flaw that you could fix to improve the library's quality, as recompense for your efforts putting it together. But you refuse to see any flaws, instead pretending that documented workarounds - not that they are even presented as such - somehow justify incorrect behaviour. You will be the death of your own project with an attitude like this.

@nyamsprod

This comment has been minimized.

Show comment
Hide comment
@nyamsprod

nyamsprod Sep 12, 2017

Member

@Bilge

  • Did I refuse to review any of your suggestions: NO
  • Did I said that any future version won't benefit from your input : NO
  • Does reading the documentation provides enough information to fix your issue : YES
  • Did you propose a PR to improve the doc : NO
  • Did you propose a PR to fix issue : NO
  • Do I find fixing this issue important enough to release a major release now : NO

And as an extra:

  • Do I find you stupid : NO
  • Did I try to have a constructive conversation with you regardless of your remarks : YES

Am I wrong maybe but as the maintainer of this package I have to make the final call on things to fix and how they will be fixed/merged.

Unless I get a PR for this, this issue will stay open and mark as enhancement for the next major release.

Member

nyamsprod commented Sep 12, 2017

@Bilge

  • Did I refuse to review any of your suggestions: NO
  • Did I said that any future version won't benefit from your input : NO
  • Does reading the documentation provides enough information to fix your issue : YES
  • Did you propose a PR to improve the doc : NO
  • Did you propose a PR to fix issue : NO
  • Do I find fixing this issue important enough to release a major release now : NO

And as an extra:

  • Do I find you stupid : NO
  • Did I try to have a constructive conversation with you regardless of your remarks : YES

Am I wrong maybe but as the maintainer of this package I have to make the final call on things to fix and how they will be fixed/merged.

Unless I get a PR for this, this issue will stay open and mark as enhancement for the next major release.

@frankdejonge

This comment has been minimized.

Show comment
Hide comment
@frankdejonge

frankdejonge Sep 12, 2017

Member

@Bilge please remember this is an open source project, people are requested to act kindly. If you come out of the gate with saying things are a "terrible design flaw" and "misleading", people are not going respond well. When this is addressed people are not "completely missing the point and taking this too personally". Maintainers can disagree with you, that does not mean they "refuse to see any flaws". I could go on, but I think the point is clear. The language you use is filled with rude comments and insults. I ask you kindly to refrain from doing so, otherwise we'll refrain from giving your issues any attention in the future.

Member

frankdejonge commented Sep 12, 2017

@Bilge please remember this is an open source project, people are requested to act kindly. If you come out of the gate with saying things are a "terrible design flaw" and "misleading", people are not going respond well. When this is addressed people are not "completely missing the point and taking this too personally". Maintainers can disagree with you, that does not mean they "refuse to see any flaws". I could go on, but I think the point is clear. The language you use is filled with rude comments and insults. I ask you kindly to refrain from doing so, otherwise we'll refrain from giving your issues any attention in the future.

@btmash

This comment has been minimized.

Show comment
Hide comment
@btmash

btmash Oct 12, 2017

Contributor

The tone in this issue is pretty...toxic to put it lightly. As a complete outsider to the project and a relative newbie to contributing to (non-Drupal) php projects, I want to say (and I really don't mean to come off as condescending/insulting and apologize if I do) :

  1. Thank you @Bilge for creating the issue. Its something that bit me as well.
  2. Thank you @nyamsprod for this project. As someone that maintains open source projects as well, I appreciate the work that goes into creating an maintaining a project in the long haul. It takes an immense amount of effort.
  3. Thank you @frankdejonge for your words. Its easy to forget that we're dealing with real people who are taking the time to make something better and online back-and-forth can make it really easy to escalate a situation out of hand.

I've added a pull request to override the static function and make a Reader class have a default 'r' open mode. Not sure if my approach is the best but am very open to making it a better out-of-the-box experience for this amazing library.

Contributor

btmash commented Oct 12, 2017

The tone in this issue is pretty...toxic to put it lightly. As a complete outsider to the project and a relative newbie to contributing to (non-Drupal) php projects, I want to say (and I really don't mean to come off as condescending/insulting and apologize if I do) :

  1. Thank you @Bilge for creating the issue. Its something that bit me as well.
  2. Thank you @nyamsprod for this project. As someone that maintains open source projects as well, I appreciate the work that goes into creating an maintaining a project in the long haul. It takes an immense amount of effort.
  3. Thank you @frankdejonge for your words. Its easy to forget that we're dealing with real people who are taking the time to make something better and online back-and-forth can make it really easy to escalate a situation out of hand.

I've added a pull request to override the static function and make a Reader class have a default 'r' open mode. Not sure if my approach is the best but am very open to making it a better out-of-the-box experience for this amazing library.

@nyamsprod

This comment has been minimized.

Show comment
Hide comment
@nyamsprod

nyamsprod Oct 12, 2017

Member

@btmash thanks for trying to resolve this issue but it has been tagged for the next major release because patching it like you did introduces a tiny BC break and since we are following SEMVER bumping to a new major version just because people don't want to read the documentation seems to be overkill. I believe even using a IDE will typehint and inform the user about the missing parameters (but I may be wrong). Anyway, I would suggested improving the documentation for now and improve the constructor signature for the next major release whenever this one will come around.

Member

nyamsprod commented Oct 12, 2017

@btmash thanks for trying to resolve this issue but it has been tagged for the next major release because patching it like you did introduces a tiny BC break and since we are following SEMVER bumping to a new major version just because people don't want to read the documentation seems to be overkill. I believe even using a IDE will typehint and inform the user about the missing parameters (but I may be wrong). Anyway, I would suggested improving the documentation for now and improve the constructor signature for the next major release whenever this one will come around.

@btmash

This comment has been minimized.

Show comment
Hide comment
@btmash

btmash Oct 12, 2017

Contributor

My problem was due to using atom which didn't typehint the default 😞

Since this has been tagged for the next major release...I guess something like this wouldn't go into master then but would it go into a separate branch or would you just keep the PR open (and we apply tags to it)? I guess I want to understand how implementing PRs for next major releases works for your project so I know how to best contribute to it.

I'll take a look at the documentation and see what I can improve on it in the meanwhile.

Contributor

btmash commented Oct 12, 2017

My problem was due to using atom which didn't typehint the default 😞

Since this has been tagged for the next major release...I guess something like this wouldn't go into master then but would it go into a separate branch or would you just keep the PR open (and we apply tags to it)? I guess I want to understand how implementing PRs for next major releases works for your project so I know how to best contribute to it.

I'll take a look at the documentation and see what I can improve on it in the meanwhile.

@nyamsprod

This comment has been minimized.

Show comment
Hide comment
@nyamsprod

nyamsprod Oct 12, 2017

Member

like stated there are no real schedule to release a new major version It depends on enhancement on PHP front or on dropping old PHP versions or coming up with new ways to improve CSV process. So the best bet is improving doc if there are not enought clear at the moment.

Member

nyamsprod commented Oct 12, 2017

like stated there are no real schedule to release a new major version It depends on enhancement on PHP front or on dropping old PHP versions or coming up with new ways to improve CSV process. So the best bet is improving doc if there are not enought clear at the moment.

@Bilge

This comment has been minimized.

Show comment
Hide comment
@Bilge

Bilge Oct 12, 2017

Please stop tagging me in this issue. @nyamsprod has demonstrated repeatedly that he does not understand this issue by iterating nonsense like this:

just because people don't want to read the documentation

Furthermore, he thinks version numbers are an endangered resource in need of conservation and would refuse to release a fix even if one was made. There is no possible way forward with someone with such limited understanding, and for that reason, this issue is terminated effective immediately.

I shall not be using this library again.

Bilge commented Oct 12, 2017

Please stop tagging me in this issue. @nyamsprod has demonstrated repeatedly that he does not understand this issue by iterating nonsense like this:

just because people don't want to read the documentation

Furthermore, he thinks version numbers are an endangered resource in need of conservation and would refuse to release a fix even if one was made. There is no possible way forward with someone with such limited understanding, and for that reason, this issue is terminated effective immediately.

I shall not be using this library again.

@Bilge Bilge closed this Oct 12, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment