Avoid magic strings for options when building Trees

[Pnoexz]

I think the project will benefit from using constants instead of magic strings when declaring options. This will allow flexibility when renaming options internally in the future, as well as avoiding all issues related to magic strings, both for people contributing to the library and those using it.

I was planning on making a PR, but I figured I should open the discussion first.

I was thinking of making a class that would looking something like this (I only added the options I'm personally using in my project):

<?php

namespace BlueM\Tree;

class Options
{
    const PARENT_KEY = 'parent';
    const ID_KEY = 'id';
    const ROOT_ID = 'rootid';
}

This would allow implementations to change like this:

         $tree = new Tree(
             $data,
             [
-                'id' => 'name',
-                'rootId' => null,
             ]
         );

         $tree = new Tree(
             $data,
             [
+                Tree\Options::ID_KEY => 'name',
+                Tree\Options::ROOT_ID => null,
             ]
         );

And the constructor method for Tree would change slightly (I don't have a diff for this because I was working locally on my vendor folder, as a proof of concept)

        if (!empty($options[Options::ID_KEY])) {
            if (!\is_string($options[Options::ID_KEY])) {
                throw new \InvalidArgumentException('Option “ID_KEY” must be a string');
            }
            $this->idKey = $options[Options::ID_KEY];
        }

This would maintain backward compatibility.

Thoughts?

[BlueM]

I don’t see the strings as a problem, as they aren’t used anywhere else. They are just something like the constructor’s public API, and as far as I remember, none of them has ever changed.

Having said that, I do think that there would be some benefit regarding IDE support if there were constants and they were named with a common prefix, such as OPT_:

Unless you feel like want to submit a PR, I’d simply do it myself, as it’s simply a matter of five to ten minutes.

[TheDigitalOrchard]

If this were ever to be updated to use a Constant-like approach, then using a native Enum would be the way to go these days.

enum TreeOptions: string
{
    case PARENT_KEY = 'parent';
    case ID_KEY = 'id';
    case ROOT_ID = 'rootid';
}