Cleaning up filesystem-related code. - Githubissues

sabre-io / katana

:hocho: A contact, calendar, task list and file server, synced, everywhere, all the time

http://sabre.io/katana/

Other

150 stars 22 forks source link

Cleaning up filesystem-related code. #281

Closed evert closed 9 years ago

evert commented 9 years ago

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 commented 9 years ago

Can we discuss about these technical choices a little bit?

evert commented 9 years ago

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 commented 9 years ago

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 commented 9 years ago

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

evert commented 9 years ago

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 commented 9 years ago

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

evert commented 9 years ago

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 commented 9 years ago

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

Hywan commented 9 years ago

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

evert commented 9 years ago

There were no methods inside that class anymore.

Hywan commented 9 years ago

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

evert commented 9 years ago

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 commented 9 years ago

Testing glob is hard. glob is slow. Please, use FilesystemIterator :smile:.

evert commented 9 years ago

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 commented 9 years ago

Fair enough.