Vinelab / NeoEloquent

The Neo4j OGM for Laravel
MIT License
633 stars 199 forks source link

[1.5-dev] Remove laravel forks #219

Closed gemini-git closed 7 years ago

gemini-git commented 7 years ago

I don't understand why there are forks of Illuminate\Support, Pagination, Contracts, Container and Events without any changes. All forks are behind master (Illuminate\Support over 400 Commits) and may contains bugs and security problems.

With this PR I changed all use-statements to the original Illuminate classes.

KinaneD commented 7 years ago

Why do you need to remove the forks? It is certainly not a coincidence that the forks are there. This is intended to achieve a Laravel agnostic NE, so that we break the dependency NE had on Laravel.

gemini-git commented 7 years ago

I understand what you mean, but it doesn't work in this case. Here some reasons:

  1. There are still dependencies for example Illuminate\Database

  2. The results of NeoEloquent functions aren't necessary compatible to other results because the forks does not share a parent class with other results (what is bad because the database driver layer should be invisible for the usage)

  3. you have side effects for example with the fork Vinelab\NeoEloquent\Support and the file helpers.php. In this file there is a new function e() witch overrides the original function of Illiuminate\Support. When you look at the function you will find the following code:

    if ($value instanceof Htmlable) {
    return $value->toHtml();
    }

    The check now only works for classes that have the class Vinelab\NeoEloquent\Contracts\Support\Htmlable as the parent class. This effect causes the problem I mentioned in #205 with the csrf_field because the original HtmlString Class isn't an instance of Vinelab\NeoEloquent\Contracts\Support\Htmlable

  4. When you fork the parts, you have to make sure that all the important changes are took over. In this case, there was not a single change since the fork, which was taken over (for example in helpers.php (see 3.) where htmlspecialchars replaced htmlentities)

This is my opinion, it is perfectly okay if you do not share the opinion. But from my point of view, it is absolutely necessary that the results in the respective version can be used without problems.

KinaneD commented 7 years ago

The intended usage of 1.5-dev is to be Laravel agnostic, whether that has been achieved or not, is as you mentioned, incomplete. This answers for points 3. and 4. since it is not supposed to be compatible with L 5.3. Honestly, i don't understand what you mean in point 2., what other results are you referring to? We intended to kill the dependency between NE and L, because we cannot keep with L's update pace while working on higher priority projects. I believe it was my mistake in the first place, to refer to this version of NE, in #196 . The last version we intended to update for L compatibility, is NE 2.0.0-alpha-plus, still not final. @Mulkave What do you think? Btw, 1.5-dev was branched out of 2.0.0-alpha-plus.

Mulkave commented 7 years ago

@geminixxl we are totally aware of the side-effects that you are mentioning, and it is one of the major reasons we haven't went public with 1.5, we will not be able to maintain the forked dependencies with their crucial updates (i.e. security) and we are actually hesitant about moving forward this way.

The main objective of 1.5 is for NeoEloquent to become framework-agnostic, and is considered as a first step in that path. We may have taking a wrong direction by forking the existing packages, is there something you suggest that will help us achieve that without having to fall into these pitfalls? Appreciate your thoughts.

gemini-git commented 7 years ago

I thought about it but I don't find any good solution. Either you have dependencies or a lot of work with crucial updates. You will always have small dependencies, as you write an extension for an existing software. In my opinion security is the most important topic, so I would choose the dependencies.

My solution would be the following: Keep the dependencies and clearify the compability with Laravel with a version number such as a.b.c.d, as I had suggested in #205. With the help of the community it should be possible to fix the compability problems for new Laravel versions without big concerns.

gemini-git commented 7 years ago

@Mulkave Any updates or decisions?

Mulkave commented 7 years ago

@geminixxl sorry for taking long on this, been trying to do some thinking around this topic. I guess it's safe to say that keeping them as dependencies is the safest way to go, forking will introduce a big amount of maintenance. Thank you for your efforts on bringing the dependencies back.