Where communities thrive


  • Join over 1.5M+ people
  • Join over 100K+ communities
  • Free without limits
  • Create your own community
People
Repo info
Activity
    Valentin Menardie
    @valanz
    Hi
    Maxime Veber
    @Nek-
    Hello everybody
    Mbechezi Mlanawo
    @Shine-neko
    hey @Nek- :D
    Maxime Veber
    @Nek-
    hi @Shine-neko
    Maxime Veber
    @Nek-
    @all what do you think about this issue ? :o https://github.com/KnpLabs/KnpMenu/issues/112
    Christophe Coevoet
    @stof
    hi
    Maxime Veber
    @Nek-
    hi @stof :)
    Maxime Veber
    @Nek-
    ping @stof can you check & merge PRs if you are ok please ? The due date for beta just pass. ^^'
    Jacek Kobus
    @jkobus
    hi all.
    Maxime Veber
    @Nek-
    Hello @jkobus
    Maxime Veber
    @Nek-
    Notice that KNPMenu is freezed because nobody check PR.
    Maxime Veber
    @Nek-
    Urgent PR to check. KnpLabs/KnpMenu#151
    Wagner Nicolas
    @n1c01a5
    hi all
    Maxime Veber
    @Nek-
    @n1c01a5 if you have a question, do not hesitate to ask, but ask, dont say hello ;) .
    or say hello and directly ask
    Maxime Veber
    @Nek-
    Hello
    Maxime Veber
    @Nek-
    We need to merge one of these 2 PR: #209 #207 what is your favorite choice ?
    Christophe Coevoet
    @stof
    Well, I don't like #209 for 2 reasons:
    • it adds overhead for everyone, even when not using weight
    • it introduces an extra dependency, and one which adds overhead in projects even when KnpMenu is not actually used (because functions cannot be lazy-loaded)
    However, I'm not a fan of putting this in the renderer as done in #207
    Christophe Coevoet
    @stof
    I'm actually wondering whether we should have separate models for the building of menus and the rendering. This would allow to let extensions hook into the conversion between them, and this would be the place where the sorting would be done
    this would have several advantages: sorting would be done only once rather than for each child being added, and the overhead would happen only when registering the extension
    it might also help in other places, for instance the matching of current items (avoiding the need to cache the computation in the matcher itself, which can leak memory)
    I need to experiment with such architectural change
    Maxime Veber
    @Nek-
    I'm ok with you. I would prefere a refactoring of the way to build items via factory. I will work on it. The fact that it comes with a new dependency is not a problem to me, but I can understand that some people don't want it, the obvious solution is to make a copy/paste of the related function inside knpmenu.
    folliked
    @folliked
    My first reaction to add sorting is that we had to change the architecture that does not really allow the hook to introduce several problems. Basically I wanted it to be triggered sorting at the final generation of the menu. Except that given the architecture that's impossible unless you make a dirty trick. I think we have to make BC break to ease the introduction of hook or plug-in menu .
    Maxime Veber
    @Nek-
    I worked a bit on refactoring the addChild method that is a problem for the feature. I had many problem with BC. After lot of brainfuck, here is the less BC Break I did: https://github.com/KnpLabs/KnpMenu/compare/master...Nek-:refactor/item-creation
    But i'm not happy with that as it's not really logical.
    And it's still a BCBreak while we are speaking about realease a 2.1 version.
    I'm in favor of merging @folliked work and refactor it inside a 3.0 version.
    ( the @folliked one as it doesn't comes with new concepts that would be useless in a 3.0 version refactored )
    @stof what do you think about ?
    And about the dependency, we could simply C/P the function we need, nop ?
    Christophe Coevoet
    @stof
    @Nek- I don't like the usage of the factory to add children this way. You are creating a huge coupling between the MenuItem and the MenuFactory as the inner working of the MenuItem is now implemented in MenuFactory. This is not a clean API
    and btw, it is not BC, so it cannot go in 2.1
    and the work of @folliked is adding a new concept (the weird in the item, and it breaks BC on the ItemInterface to add this concept)
    And given it adds overhead in all cases, and most people won't need it (as none of the existing menus built with the library are using a weight) but still suffer for the performance overhead
    it would make building menus slower to implement an edge case
    #207 is not the cleanest way to solve the issue, but this is the one not introducing new concepts (it can totally be implemented as a separate package for now without requiring any change in KnpMenu)
    Maxime Veber
    @Nek-

    @stof aw yes, I didn't break the BC, but it should be done in a new version of knpmenu (I added a new arg on createItem). But as you say, this implementation couple the factory to the item and i'm not happy with that either, the fact is that I couldn't make something better without breaking things. Do you have something to refactore without breaking things ? (I was thinking about using a factory extension to manage the weight)

    About the break in @folliked 's PR, I didn't notice it, so we should merge #207 . IMO it's not a good idea to fragment knpmenu in many repo (and as it's needed for complete integration, it adds a new dependency for KnpMenuBundle).

    Christophe Coevoet
    @stof
    @Nek- your branch is breaking BC. You are adding a method in an interface
    And what I mean is that I'm not sure we should add #207 in the core given that it is a weird implementation for the feature. So the author could keep its own package providing such feature at the renderer level for now until we can find an architecture allowing a clean implementation
    I have an idea about it, but I don't have time to experiment with it in the following days
    Maxime Veber
    @Nek-
    @stof ok for releasing only with what we have now a new 2.0.2 version ?
    Christophe Coevoet
    @stof
    @Nek- I would like to make a review of existing PRs to see what is mergeable before the release. I will try to do it this evening