Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Mark the extension as deprecated #15

Merged
merged 1 commit into from Feb 8, 2016
Merged

Mark the extension as deprecated #15

merged 1 commit into from Feb 8, 2016

Conversation

nitriques
Copy link
Member

See #14 for details

@brendo Do I need to create a new version that explains why and to point to Select Box Link ?

cc @nilshoerrmann @twiro @kmeinke

@nitriques nitriques changed the title Make the extension as deprecated Mark the extension as deprecated Dec 9, 2014
@nilshoerrmann
Copy link

The question is, if SBL or AF is the correct replacement. It makes sense to link to the latter because it includes the interfaces – additionally, the original idea was to deprecate SBL in favour of AF as well because AF is just SBL plus the interface hooks.

I'll leave this one to @brendo.

@twiro
Copy link
Member

twiro commented Dec 9, 2014

I think a note in the readme, pointing to "Association UI: Selector" as an alternative for the autocomplete functionality would be helpful.

And maybe setting the maximum compatibility to 2.3.2.

@twiro
Copy link
Member

twiro commented Dec 9, 2014

Ah - Nils was faster than me :)

additionally, original idea was to deprecate SBL in favour of AF as well because AF is just SBL plus the interface hooks.

That's an important point and the next thing I wanted to "complain" about ;)
AF coexisting to SBL doesn't make much sense to me and I always understood AF as a replacement for SBL. But the current state of AF isn't really satisfying as it not only hasn't replaced SBL yet but includes bugs that have been solved for SBL and isn't even listed on symphonyextensions.com.

But that doesn't belong here... (though it's directly related to why Reference Field should be deprecated)... gonna have to write about that elsewhere (as soon as I find the time).

@nilshoerrmann
Copy link

The pending issue is here: symphonists/association_field#5

@michael-e
Copy link
Member

AF coexisting to SBL doesn't make much sense to me and I always understood AF as a replacement for SBL. But the current state of AF isn't really satisfying as it not only hasn't replaced SBL yet but includes bugs that have been solved for SBL and isn't even listed on symphonyextensions.com.

A decision on this topic would be more than welcome. I can not really help with this, since I don't know how much additional code has been introduced by AF, or if adding it to the "core extensions" (and deprecating SBL) is actually desired by @brendo. Anyway, in this case it must be guaranteed that replacing the field is painless under all circumstances.

@nitriques
Copy link
Member Author

Anyway, in this case it must be guaranteed that replacing the field is painless under all circumstances.

Trudat.

@nilshoerrmann
Copy link

AF is really just a fork of SBL with two additional field settings that are only available if you install one of the interfaces. If you don't, the settings are hidden and AF looks and behaves exactly like SBL. It's the same code – AF like SBL doesn't include any UI by itself (okay, the default select box, of course). These are all changes made on top of SBL: https://github.com/symphonists/association_field/compare/b2c9ed72a7b044c146171be2e40041752181e022...symphonists:master?w=1

This is how you update:

  1. Install Association Field
  2. Rename sym_fields_selectbox_link to sym_fields_association.
  3. In sym_fields rename all selectbox_link types to association.

Currently, AF is behind SBL regarding bug fixes, because symphonists/association_field#5 is still open and fixes to SBL have not been merged back to AF yet.


PS: Updated the link to the code comparision

@nilshoerrmann
Copy link

… if adding it to the "core extensions" (and deprecating SBL) is actually desired by @brendo.

Well, at least Brendan agreed on that in a chat before we started working on AF. Association is the name used by Symphony internally and the new extension name was chosen to make clear that the field can be used to create any kind of relationship – the interfaces are a just a visual and interactive bonus which is why they reside in separate extensions.

As Brendan takes care of nearly everything around Symphony, I leave the decision to him. But having both extensions around only complicated things: we should merge both and deprecate one of them. I'd prefer keeping AF because of the naming, but in the end it actually doesn't matter because the codebase is the same.

@nitriques
Copy link
Member Author

@nilshoerrmann So referencelink should really be deprecated then... And we should deprecate SBL soon too...

@twiro
Copy link
Member

twiro commented Dec 9, 2014

AF is really just a fork of SBL with two additional field settings that are only available if you install one of the interfaces. If you don't, the settings are hidden and AF looks and behaves exactly like SBL.

What about the possibility of storing the sort-order of associated entries? Isn't AF capable of that "by core (=additional field in the DB)" whereas SBL isn't? Or is storing the sort-oder just "injected" by the association UI?

the interfaces are a just a visual and interactive bonus which is why they reside in separate extensions.

...which is one of the reasons why I still think treating the "Selectbox UI" the way I proposed here would help understanding and be the cleaner solution in the long run.

@nitriques
Copy link
Member Author

Yeah order is why entry relationship field does not extends AF...

@nilshoerrmann
Copy link

What about the possibility of storing the sort-order of associated entries? Isn't AF capable of that "by core (=additional field in the DB)" whereas SBL isn't? Or is storing the sort-oder just "injected" by the association UI?

Both fields, SBL and AF, are capable of storing sorted associations out of the box: the order in which associations are posted to the server is decisive. The extensions just lack a default interface for sorting – this is what the association UI is for.

@nitriques
Copy link
Member Author

the order in which associations are posted to server is decisive.

I think this is a problem: what happens if I have 50, 100 selected values ? SBL currently deletes 50 rows and re-insert 50 rows... not that performant.

@nilshoerrmann
Copy link

I think this is a problem: what happens if I have 50, 100 selected values ? SBL currently deletes 50 rows and re-insert 50 rows... not that performant.

Well, that's a different topic. It's just the way it currently works in SBL.

@nitriques
Copy link
Member Author

Yeah I know 😄 Just wanted to share my thoughts on it

@twiro
Copy link
Member

twiro commented Dec 9, 2014

Both fields, SBL and AF, are capable of storing sorted associations: the order in which associations are posted to server is decisive. The extensions just lack a default interface for sorting – this is what the association UI is for.

That's good to know :)

So if I get this right I could manually sort an association field with one UI (that has sorting functionality), then switch to another UI and my manual sort-order isn't affected from that change?

@nilshoerrmann
Copy link

Correct.

@twiro
Copy link
Member

twiro commented Dec 9, 2014

Correct.

Perfect :)

Now back to work...

@kmeinke
Copy link

kmeinke commented Dec 9, 2014

@nilshoerrmann Your how to update selectbox_link fails for reference_link.
reference_link and selectbox_link may be alike but not the same. The table schema of the entry table for a reference_links with autocomplete dos not contain the relation_id - but the handle, and value of the referenced field.

I sugest a better upgrade guide for reference link to Association Field could be:

  1. backup
  2. Install Association Field
  3. append the relation_id in the entries_data table (for autocomplete referencelinks only)
    • add column relation_id (int) to sym_entries_data_XXX
    • Update sym_entries_data_XXX AS parent
      Inner Join sym_entries_data_YYY AS child ON child.handle = parent.handle
      Set parent.relation_id = child.entry_id
  4. Update sym_fields_association with sym_fields_referencelink. set missing columns to defaults
  5. In sym_fields rename all referencelink types to association
  6. Disable Referencelink
  7. Test (tests will fail before 5!)

XXX is the field_ID of the old reference_link
YYY is the field_ID of the referenced field
both values can be found in sym_sections_association

...but we should provide a script to upgrade that - some pseudo code for that:
https://gist.github.com/kmeinke/53d28d3bdbf1832c1954

@kmeinke
Copy link

kmeinke commented Dec 9, 2014

btw... there is a lot redundancy between sym_sections_association and sym_fields_association.
I do understand why - but still... perhabs associations between sections should not be an extension at all?

@twiro
Copy link
Member

twiro commented Dec 9, 2014

The table schema of the entry table for a reference_links with autocomplete dos not contain the relation_id - but the handle, and value of the referenced field.

I guess you are looking at the wrong table ;)
One that stores values Reference Link gets populated with, not the one that stores the actual association.

Have a look at the sym_fields_referencelink-table, take an ID you find listed under field_id (not related_field_id) and check the corresponding sym_entries_data_XXX-table. That's there the relation/association is stored using the same structure (id, entry_id and relation_id) as SBL.

@kmeinke
Copy link

kmeinke commented Dec 9, 2014

Maybe - i am a newbie in your code :) - but i am quite sure, cause I updated it today and it works :) i'll check tomorrow if i must be sorry... again. gn8

@nitriques
Copy link
Member Author

So what's the end result on this ? We deprecate it ? WIth 2.6.0 coming out soon, it think it's the time.

nitriques added a commit that referenced this pull request Feb 8, 2016
Mark the extension as deprecated
@nitriques nitriques merged commit 85d62de into master Feb 8, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants