[WIP] Initial Component Structure #1

hernandev · 2017-02-07T01:33:53Z

WORK IN PROGRESS - DO NOT MERGE
This is a separate PR to define a component structure, actual code will be sent on a second PR

DennyLoko · 2017-02-07T10:53:04Z

What are the methods we are expecting this PR to implement? Can you list them as a to-do list so we can track the progress and suggest changes based on it?

Thanks!

naroga · 2017-02-07T11:14:24Z

.php_cs

@@ -0,0 +1,18 @@
+<?php


I don't think this should be versioned.

.php_cs.dist should, is a simple way to enforce code style, this one don't need to versioned indeed

naroga · 2017-02-07T11:17:49Z

src/Contracts/Str.php

@@ -0,0 +1,46 @@
+<?php


Str should be a primitive type. I don't see much value in making it implement a interface, as there shouldn't be multiple Str implementations. Specially so when this is only a wrapper for standard functionality.

Maybe if we intend to implement both immutable (which in this context should be akin to constant) strings and mutable strings, it would make sense to have them both implement a contract. Maybe I don't care if my string is mutable. I'm not sure if it makes sense.

I think we should support both, as suggested here, but reinforce the immutable state by suffixing the class.

What is the goal of having immutable strings? What do we expect to accomplish with it?

I understand keeping track of object state is important, and immutability helps in that aspect. But if an object is a primitive type wrapper, is its state important?

As a side note, primitive types are always passed by copy in PHP, where objects are always passed as references. In this aspect, immutable objects replicate best a primitive type, as any modification will not propagate everywhere. The performance loss greatly concerns me, though. Benchmarks would be greatly appreciated here.

@naroga if we're using as objects, we cannot copy the variable without clone. Remember they are objects so if we do a toUpperCase() on the variable, it will have undesired effects if not immutable

For the record, this discussion has been settled in issue #4.

naroga · 2017-02-07T11:19:30Z

src/Str.php

+    /**
+     * @var string Raw string content
+     */
+    protected $payload;


I'd rather have this be named content, contents or something else. Personal preference, only. Payload reminds me of multitype data, blobs, text, etc. We're talking about text-only data.

naroga · 2017-02-07T11:22:33Z

src/Str.php

+     *
+     * @return StrContract Object instance with the replaced text.
+     */
+    public function replace($search, string $replace)


I strongly disagree with using a factory to run a replace here.

This wrapper should be as performant as possible, and it should do this by avoiding instantiation of redundant objects. Adding a big footprint just to OOPize things will greatly hinder this library's adoption. With that in mind, mutable objects should be preferred, as they wouldn't require new objects to be instantiated on replace and others.

I think, at least for the "primitive components" (string, number and maybe array), we should use immutable objects. Immutability achieves a defensive way of programming by preventing objects to change their state, which is a preference for some developers. Also, it's the same implementation of the native PHP functions, making the transition from the existing code to PHP-OL more "natural".

I also think this should be discussed in project-level.

@marcossffilho If our goal is to port the primitive types to a OO fashion with as little footprint as possible, then immutability is a moot point. The primitive types are mutable themselves, and changing this behavior by default breaks the "porting" aspect of the library. PHP strings are mutable, but our strings won't be?

Also, immutability costs more memory and CPU to perform operations, as new instances have to be created.

$myString = Str("my name is pedro"); //First and only variable assignment $myString->replace("pedro", "naroga"); echo $myString;

In the previous snippet, if Str is immutable, it will print my name is pedro, instead of the expected my name is naroga. In an immutable way of coding, we'd need to apply the follow reset to the variable:

$myString = Str("my name is pedro"); //First variable assignment $myString = $myString->replace("pedro", "naroga"); //Second variable assignment, which is not good echo $myString;

Immutability brings little to no value to primitive types, as I presented in my other comment. It brings value to objects that need to be state-tracked. Primitives don't need to have their states tracked.

However, if Str is mutable, the first snippet would work as expected: my name is naroga.

Note that even if we opt to make the object immutable, the variable itself is still mutable, and in the second snippet, I actively changed $myString. The object is immutable, but the variable isn't.

I will repeat: when object state needs to be tracked (and they need to be tracked most of the time), immutability helps. When object state doesn't need to be tracked (and primitives don't need their state tracked), immutability is moot. A wrapper for a primitive should also not need to rely in its state, only in its content value. Other data types (Set, LinkedList, Dictionaries) may benefit from immutability, but String, Number, Array certainly don't.

I think Str should be mutable by default (for performance and compliance with the primitive php type), but we could optionally have a ImmutableStr or StrImmutable.

Also noting that Immutable here would have the same value that of const * has in C/C++ parameters (readonly references that can't be changed). It would bring some value to the language, as primitive strings that travel through many different scopes in PHP are copied (and thus, local changes would not propagate elsewhere). It would be a "change prevention", that best emulates the passing-by-value of a primitive string -- but instead of discarding changes on scope change, it would prevent changes.

I am not so sure with "PHP strings are mutable" because they are primitives. Primitives have no state, they're just a value. When you change a string variable, you don't mutate the value, you just replace it with a new one. But when I said that immutability is the same implementation of native PHP functions, I refer to the fact that common string functions like str_replace return a new value.

I understand the point about performance. And also agree if we're going to have both "Str" and "StrImmutable" (I don't like the ImmutableStr name).

Another point that I want to show is template strings:

<?php $tpl = new Str('[Person] love PHP-OL!'); foreach (['naroga', 'marcossffilho', 'hernandev', 'everyone'] as $person) { echo $tpl->replace('[Person]', $person); }

An example on string mutability:

$myString = "pedro"; $myString[2] = "a"; echo $myString; //prints "pearo";

I have created a string, and made modifications to a subset of the string.
In a similar fashion, our wrappers should have no state, only value. Having no state, being immutable makes no sense.

In an analogous fashion, Str('pedro')->modifyCharAt('2', 'a'); or something to that effect should modify the Str content.

I want to add this use-case for discussion (if it's relevant):

<?php $str = new Str('a string'); $start = $str->slice(0, 3); $end = $str->slice(-3);

naroga · 2017-02-07T11:23:28Z

composer.json

+    },
+    "require-dev": {
+        "phpunit/phpunit": "^6.0",
+        "friendsofphp/php-cs-fixer": "^2.0"


Likewise, cs-fixer should be something we all have installed in our dev environments (something like phpcbf), not a part of the project, IMO.

Should we fix a PHP version via composer?

naroga · 2017-02-07T13:16:58Z

src/Str.php

+     */
+    public function replace($search, string $replace)
+    {
+        return $this->factory(str_replace($search, $replace, $this));


Very important!

Please note that str_replace doesn't work with any UTF-8 characters. If we are to encapsulate string methods here, we should try to use mb_string functions whenever possible and fallback to the default str_replace, if mb_string is not available.

Oriental (japanese, chinese, korean, arabic, etc) characters do not work with str_replace!

Or, we can also require ext-mbstring via Composer

@marcossffilho I don't need mbstring to work with most projects, and most shared hosts don't offer the option to install extensions. I'd rather do the "symfony way", and check if the extension is loaded; if not, fall back to the default.

I don't mind the verification, but IMHO mbstring is a pretty common extension, never saw a shared host that doesn't have it.

filhocodes · 2017-02-09T00:25:49Z

src/Str.php

+     *
+     * @param string|StrContract $payload The actual string
+     */
+    public function __construct(string $payload)


Should we have static factories, so we can call e.g. Str::from($primitive)->replace instead of (new Str($primitive))->replace?

Or default for all, Str::new as example

tsantos84 · 2017-02-13T17:36:07Z

src/Str.php

+     */
+    protected function factory(string $str)
+    {
+        return new self($str);


Return new static($str); is better, IMO? You can create instances regardless the subtype that is calling Str::factory method.

Diego Hernandes added 8 commits February 6, 2017 23:26

composer initial config

2f62a5e

phpunit added as dependency

6ccf869

initial phpunit configuration file (dist)

0cb16b7

php-cs-fixer config file

caf2a04

php-cs-fixer as dev dependency and initial test

306f462

initial method replace / 100% coverage

80f0b09

cs-fixer

273e0d5

new test passing object

76ee32b

naroga reviewed Feb 7, 2017

View reviewed changes

DennyLoko mentioned this pull request Feb 8, 2017

String Component phpol/phpol#9

Closed

filhocodes reviewed Feb 9, 2017

View reviewed changes

filhocodes mentioned this pull request Feb 9, 2017

String Formatters #2

Open

tsantos84 reviewed Feb 13, 2017

View reviewed changes

[WIP] Initial Component Structure #1

Are you sure you want to change the base?

[WIP] Initial Component Structure #1

Uh oh!

Conversation

hernandev commented Feb 7, 2017

Uh oh!

DennyLoko commented Feb 7, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

naroga Feb 7, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DennyLoko Feb 7, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

naroga Feb 7, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

naroga Feb 7, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

naroga Feb 7, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

naroga Feb 7, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

filhocodes Feb 9, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

filhocodes Feb 9, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

naroga Feb 7, 2017 •

edited

Loading

DennyLoko Feb 7, 2017 •

edited

Loading

naroga Feb 7, 2017 •

edited

Loading

naroga Feb 7, 2017 •

edited

Loading

naroga Feb 7, 2017 •

edited

Loading

naroga Feb 7, 2017 •

edited

Loading

filhocodes Feb 9, 2017 •

edited

Loading

filhocodes Feb 9, 2017 •

edited

Loading