Refactor and rename #221

mikey179 · 2020-02-25T15:40:56Z

As discussed in #213 I took the opportunity to rename some classes. Also, in #220 (review) I mentioned I believe there's opportunity to restructure code. This is my attempt at that. It probably has some rough edges and I'm not sure it's a good idea to merge this big one. On the other hand, this is a big opportunity, as we will have a new major version anyway, and I managed to reduce the amount of code while all features are kept. Please have a look and let me know what you think.

mikey179

I added some comments throughout the diff to highlight some things I think are worth to mention or to discuss.

mikey179 · 2020-02-25T15:51:19Z

src/Inode.php

 */
-abstract class vfsStreamAbstractContent implements vfsStreamContent
+class Inode


Like in a real filesystem , the inode contains all metadata about the file, except the filename and some other things that are specific to the type of file.

I really like this

mikey179 · 2020-02-25T15:52:34Z

src/StreamWrapper.php

@@ -141,7 +94,7 @@ class vfsStreamWrapper
     */
    public static function register(): void
    {
-        self::$root = null;
+        self::$root = Root::empty();


I introduced a specific Root class so that some null checks could be eliminated. I believe this could be a starting point should we ever want to tackle the topic of partitions.

mikey179 · 2020-02-25T15:56:22Z

src/content/FileContent.php

     */
-    public function seek(int $offset, int $whence): bool;


I removed all seeking functionality from the content instances itself. Instead, this functionality can now be found in internal\OpenedFile, for which a new instance is created on each fopen(). This resembles what @bizurkur achieved with #220.

This took me a minute to get used to. It's neat that it avoids the extra seeks before each read/write in OpenedFile.

src/internal/ErroneousOpenedFile.php

mikey179 · 2020-02-25T15:59:00Z

src/internal/Mode.php

+ *
+ * @internal
+ */
+final class Mode


Just some stuff extracted from the StreamWrapper class to reduce its size.

mikey179 · 2020-02-25T16:14:29Z

src/BasicFile.php

+     *
+     * @return int[]|false
+     */
+    public function stat()


Putting that here eliminates some code duplication in the streamwrapper class.

mikey179 · 2020-02-25T16:18:00Z

src/Inode.php

@@ -237,9 +169,19 @@ public function chmod(int $permissions): vfsStreamContent
    }

    /**
-     * returns permissions
+     * @deprecated  use permissions() instead


Just a suggestion, like with all getX() methods which return just the value of a property.

Maybe it's just me, but I like the get/set prefixes because it makes it very obvious what the method does.

I might just be lazy and not like reading a method's description to find out what it's doing. An example:

$file->lastAttributeModified($filectime); // takes parameter = setter $file->filectime(); // no parameter = getter // but then... $file->resourceId($resource); // takes parameter, but is a getter not a setter

You are right, resourceId($resource) is not a good method name for what it does. Should be something like resourceIdOf($resource) or resourceIdFor($resource) I think.

src/internal/Path.php

Co-Authored-By: Luke <bizurkur@users.noreply.github.com>

…the latter doesn't need to extend the former without using any of its functionality

src/internal/ErroneousOpenedFile.php

mikey179 · 2020-02-28T09:00:34Z

In my last commit I updated the org layer to load the new classes. However, I'm a bit unsure how to continue:

Remove all classes/interfaces from org which don't have a match any more? These are mostly ones which people typically shouldn't have used, but you never know... but it would break the current promise that an update can be done seamlessly.
Does the org layer actually still make sense with the renaming?
Or should it be two steps - a release 2.0 with the namespace change only, quickly followed by a 3.0 with the rename in place?

I'd like to hear your thoughts on this.

bizurkur · 2020-02-28T12:53:26Z

Does the org layer actually still make sense with the renaming?

Or should it be two steps - a release 2.0 with the namespace change only, quickly followed by a 3.0 with the rename in place?

I was a little curious about this, too. The rename does seem like it makes the old org layer somewhat pointless if all the files don't match up anymore. If the goal is to maintain an easy transition, putting this into 3.0 does sound like a better idea.

If we go that route, then we can update the org files that don't have a match anymore to include "this file will be removed in 3.0" to the deprecation message.

Then for any development, we'll just have to create a 2.x and remember to branch off of that. Merging any 2.x changes into 3.x could be fun...

mikey179 · 2020-02-28T14:04:49Z

Merging any 2.x changes into 3.x could be fun...

Yes, that's one of the thoughts I had - we might create a maintenance problem for us. So in case we opt for two major releases (not necessarily at the same time) we should limit the support time span for 2.0 - I'd even go to say that it should receive bug fixes only.

mikey179 · 2020-03-02T09:24:40Z

Over the weekend I thought a bit about this and came up with the following idea:

Merge the namespace change into 1.x.
Rename the classes that will receive a new name, adjust the org namespace accordingly to reference the new names, and to declare the classes/interfaces which will be removed because they are not present any more after the refactoring.
Release this as 1.7.0.
Remove the org layer with the old namespace from master.
Merge this one into master, and continue the work with the other issues and open PRs.
Once we deem master ready for release it becomes 2.0.0.

This has some advantages:

People can upgrade to 1.7 without any problems, and start to adjust their code in preparation for 2.0. But they don't have to do this immediately with the upgrade.
Once people did the changes that become possible with 1.7 an upgrade to 2.0 should not require any major work.
We decouple our further work on 2.0 from the namespace change.
There's only one major release on the time plan, not two as with my initial thoughts.

What do you think? Have I overlooked anything that might pose a problem?

bizurkur · 2020-03-02T12:48:19Z

That sounds way better and should have far fewer complications 👍

This ports the namespace change introduced with #209 to the 1.x series, as suggested in #221. This allows users to upgrade to 1.7 and adjust the used namespace.

port #209 to 1.x as suggested in #221 This ports the namespace change introduced with #209 to the 1.x series, as suggested in #221. This allows users to upgrade to 1.7 and adjust the used namespace. * raise requirement for minimum PHP version to 5.6.0 * remove builds with PHP < 5.6

This introduces the class name changes proposed with #213 into the v1.x series according to the migration strategy detailed in #221.

Also update changelog.

* introduce class name changes in new namespace This introduces the class name changes proposed with #213 into the v1.x series according to the migration strategy detailed in #221. * fix various type hints

jaapio · 2020-07-03T12:44:03Z

What is the status of this pr? I would like to start working on php8 compatibility. But this large move of files makes me feel that I have to wait for this PR.

I reviewed the changes, I think this one is good to go...

allejo · 2020-07-03T22:25:24Z

Agreed. What else is left in this PR?

I'd like to get a migration path from 1.7.0 to 2.0.0 so we can finally EOL the 1.x series.

mikey179 · 2020-07-06T11:35:00Z

If I remember correctly, within the plan I outlined above this should now be at point 3. Unfortunately for this PR I got pulled into a large project mid March and are lacking any time to continue working on this. I'm not sure if this one could be merged as is or if there were some other changes that should be done, something in the back of my mind tells me I wanted to look at something but I can't remember what it was. Probably it's ok to check with #227 if something needs to be done here before it should be merged so the change is consistent in 1.x and master.

mikey179 added 5 commits February 25, 2020 16:32

refactor and rename

06139c8

fix some phpstan issues

64e1b8d

return to previous notation

ab527c1

return to previous notation

688c395

remove leftover from debugging

c4535b2

mikey179 commented Feb 25, 2020

View reviewed changes

bizurkur mentioned this pull request Feb 25, 2020

Cannot read and write to open streams on the same file #222

Open

bizurkur reviewed Feb 26, 2020

View reviewed changes

src/internal/Path.php Outdated Show resolved Hide resolved

mikey179 and others added 9 commits February 26, 2020 09:58

remove trailing comma

fe50757

Co-Authored-By: Luke <bizurkur@users.noreply.github.com>

add comment

6a348e2

uncomment, was for testing purposes only

4d72430

adjust to renamed classes

97d1497

automatic fix of coding style violations with phpcbf

bf2acc4

fix case

bed6a3a

add missing type hint

d6804c6

get rid of Abstract in class name, use a better name

c92dc22

introduce common interface for OpenedFile and ErroneousOpenedFile so …

53fbb70

…the latter doesn't need to extend the former without using any of its functionality

mikey179 commented Feb 26, 2020

View reviewed changes

src/internal/ErroneousOpenedFile.php Show resolved Hide resolved

IMSoP mentioned this pull request Feb 26, 2020

Fix for Issue #222: handle EOF and offset correctly on read #224

Open

mikey179 added 3 commits February 27, 2020 10:44

use fixed phpcs action

c9eac41

add proper description for parameter

9ba554b

use renamed classes where they exist

a27c041

mikey179 requested a review from a team February 28, 2020 09:01

mikey179 added a commit that referenced this pull request Mar 3, 2020

port #209 to 1.x as suggested in #221

d9aec4e

This ports the namespace change introduced with #209 to the 1.x series, as suggested in #221. This allows users to upgrade to 1.7 and adjust the used namespace.

mikey179 mentioned this pull request Mar 3, 2020

Port #209 to 1.x as suggested in #221 #226

Merged

mikey179 added a commit that referenced this pull request Mar 5, 2020

introduce class name changes in new namespace

8f117f5

This introduces the class name changes proposed with #213 into the v1.x series according to the migration strategy detailed in #221.

mikey179 mentioned this pull request Mar 5, 2020

Introduce class name changes in new namespace #227

Merged

remove org namespace and migration layer

8a9f28f

Also update changelog.

bizurkur mentioned this pull request Jul 10, 2022

Allow for custom directory separators #275

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor and rename #221

Refactor and rename #221

mikey179 commented Feb 25, 2020

mikey179 left a comment

mikey179 Feb 25, 2020

bizurkur Feb 26, 2020 •

edited

Loading

mikey179 Feb 25, 2020

mikey179 Feb 25, 2020

bizurkur Feb 26, 2020

mikey179 Feb 25, 2020

mikey179 Feb 25, 2020

mikey179 Feb 25, 2020

bizurkur Feb 26, 2020

mikey179 Feb 26, 2020

mikey179 commented Feb 28, 2020

bizurkur commented Feb 28, 2020

mikey179 commented Feb 28, 2020

mikey179 commented Mar 2, 2020

bizurkur commented Mar 2, 2020

jaapio commented Jul 3, 2020

allejo commented Jul 3, 2020

mikey179 commented Jul 6, 2020

Refactor and rename #221

Are you sure you want to change the base?

Refactor and rename #221

Conversation

mikey179 commented Feb 25, 2020

mikey179 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bizurkur Feb 26, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mikey179 commented Feb 28, 2020

bizurkur commented Feb 28, 2020

mikey179 commented Feb 28, 2020

mikey179 commented Mar 2, 2020

bizurkur commented Mar 2, 2020

jaapio commented Jul 3, 2020

allejo commented Jul 3, 2020

mikey179 commented Jul 6, 2020

bizurkur Feb 26, 2020 •

edited

Loading