Skip to content
This repository has been archived by the owner on Aug 12, 2019. It is now read-only.

Cleaning up filesystem-related code. #281

Merged
merged 7 commits into from
Oct 20, 2015
Merged

Cleaning up filesystem-related code. #281

merged 7 commits into from
Oct 20, 2015

Conversation

evert
Copy link
Member

@evert evert commented Jul 5, 2015

This patch intends to remove:

  • Everything related to the katana:// stream wrapper.
  • Everything that uses Hoa\File that can be easily replaced by PHP's plain filesystem wrappers.
  • For now it also removes the exception handlers. I don't think they work particularly well and they add a few non-idiomatic constructs. In the development phase I believe we are better off just not catching exceptions, and address in the future again where this might cause us issues.
  • Removed the Database class, in favor of using PHP's PDO class.
  • Moved the version constants to their own class, to make it consistent with other sabre projects.

This is a work in progress.

See #277

@Hywan Hywan self-assigned this Jul 5, 2015
@Hywan
Copy link
Member

Hywan commented Jul 5, 2015

Can we discuss about these technical choices a little bit?

@evert
Copy link
Member Author

evert commented Jul 5, 2015

Sure! I know this must be a bit frustrating to you, but we need to simplify. Abstracting the filesystem might have given some marginal gain, but it's been at the cost of an unfamiliar and hard to debug API.

@Hywan
Copy link
Member

Hywan commented Jul 5, 2015

From my point of view, we have just one bug with a not very helpful error message. Could we just fix that and wait for another hard to understand bug to happen before taking these decisions? katana:// is very helpful…

@Hywan
Copy link
Member

Hywan commented Jul 5, 2015

And I am afraid we introduce a new regression with these changes. The bug #277 is already a regression.

@evert
Copy link
Member Author

evert commented Jul 5, 2015

There's other benefits to having less code and less ivanisms ;) its not the first bug, and it won't be the last.

But the bigger issue is that I see 0 benefit to using in the first place, and it adds several layers if indirection to tasks that are normally pretty straightforward. I've yet to hear the first argument why it was ever benefitial

@Hywan
Copy link
Member

Hywan commented Jul 5, 2015

Let me review your PR then. Regression will be pretty easy to introduce.

@evert
Copy link
Member Author

evert commented Jul 5, 2015

Thank you! I was not fully done though. Got sleepy before I did real testing, so you could also wait till I did that

@Hywan
Copy link
Member

Hywan commented Jul 5, 2015

Sure. Ping me when it's ready for a review :-).

@Hywan
Copy link
Member

Hywan commented Jul 5, 2015

Oh, and why removing Sabre\Katana\Database? There is no bug inside it and it's pretty useful :-/.

@evert
Copy link
Member Author

evert commented Jul 5, 2015

There were no methods inside that class anymore.

@Hywan
Copy link
Member

Hywan commented Jul 5, 2015

Except the iterator :-), which is very useful.

@evert
Copy link
Member Author

evert commented Jul 5, 2015

From a pure functionality standpoint the exact same thing is happening now. The difference is that we use a PHP built-in. I can see that it could be useful to move that glob() statement to its own function, but it doesn't really need to be a method on a 'Database` class. It's really a separate concern.

@Hywan
Copy link
Member

Hywan commented Jul 6, 2015

Testing glob is hard. glob is slow. Please, use FilesystemIterator 😄.

@evert
Copy link
Member Author

evert commented Jul 6, 2015

We're iterating over 5 files that nearly never change. There's no reason to make this more complicated than this is. If we need complexity later, we can add it then.

@Hywan
Copy link
Member

Hywan commented Jul 7, 2015

Fair enough.

evert added a commit that referenced this pull request Oct 20, 2015
Cleaning up filesystem-related code.
@evert evert merged commit 4e911e4 into master Oct 20, 2015
@Hywan Hywan removed the in progress label Oct 20, 2015
@evert evert deleted the no-stream-wrappers branch October 20, 2015 22:31
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.

2 participants