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

Maintainer update. Thoughts on modernizing. #144

Open
tetranz opened this issue Jun 17, 2019 · 10 comments
Open

Maintainer update. Thoughts on modernizing. #144

tetranz opened this issue Jun 17, 2019 · 10 comments

Comments

@tetranz
Copy link
Owner

tetranz commented Jun 17, 2019

Hi all

I'm the maintainer of this project. I know it might have looked like it over the last few years but I haven't abandoned it. It's just been a busy few years with little spare time. 😄 When I started this almost five years ago I didn't expect it to get popular. If the current rate keeps up it will have half a million downloads on Packagist soon.

Thanks to all who have contributed PRs and sorry for my lack of response. I hope to be a bit more active in future. I merged a few easy PRs today and released 2.9.6. The other open PRs will need some more attention.

Yesterday I spent some time playing with it on Symfony 4 configured to use Webpack. It was quite easy to get working but I wonder if we need to modernize it a little.

I got it working by adding the following to my template
<script src="{{ asset('bundles/tetranzselect2entity/js/select2entity.js') }}"></script>
I also added the Select2 CDN tags my template.

This works but it required the following in app.js

const $ = require('jquery');
global.$ = global.jQuery = $;

I don't think we need the $ but it would probably be nice to avoid the global. It would also be nice for our JS code to be aggregated into app.js.

I probably need some help on the best way to do this and/or opinions on whether we really need to do this. I'm not sure how to make it a node module without breaking it for other users. I'm not a front end developer so some of this is a bit out of my normal experience.

There is lots of other modernizing we could do. Should we convert ES6 with arrow functions etc? I don't want it to get too far behind but on the other hand I'm conscious of "if it ain't broke don't fix it".

@DubbleClick
Copy link
Contributor

DubbleClick commented Jul 17, 2019

Hey, first off thanks for the bundle. As it lacked some functionality and I wasn't too sure on whether you planned Symfony 4 updates or not, I decided to include the files into my project instead and rewrite some parts. It's been a while and I can't currently see the project, but I believe to remember some of the changes.

This works but it required the following in app.js

const $ = require('jquery');
global.$ = global.jQuery = $;

I didn't have to do this. Unsure why you have to, considering the function is called with jQuery as parameter?

Should we convert ES6 with arrow functions etc? I don't want it to get too far behind but on the other hand I'm conscious of "if it ain't broke don't fix it".

Unnecessary in my opinion. If you get around to it then sure, why not, but I think that's of least importance. Some modernizing is due though, a flex recipe would be nice, making the js code a node module, adding some functionality.

I remember having to make some changes to fields.html.twig for the select2ajax_widget, but I can't really remember what those were, one to get it working, one to add some extra parameters and a last one to allow styling of entries (not possible with the tag, for all I know). I think there was also a treebuilder deprecation for DI. And that you had to manually include the language pack of your choice. For some reason minimum character length was broken for me as well, if I ran npm prod. I can't remember how I solved that. And there was an issue with the width of the select2 ajax dropdown being wider than other entity fields - but that might've only been an individual problem.

Aside of that, I think some people will need more parameters to be sent with the ajax call to the server. That could obviously be solved by creating multiple routes to use at different places, but it would litter the whole thing unnecessarily.

If I get around to it, I might apply some of my changes to a PR and switch to using the bundle instead, if it's actively maintained for S4.

@n3o77
Copy link
Contributor

n3o77 commented Jul 18, 2019

Thanks for creating this bundle and still maintaining it @tetranz !

Just a few thoughts of mine and nice to have things:

I think proper Symfony 4 support would be nice. Modernize the codebase a bit. i.E.: Use the class name as service id and set it to public so auto injection works. Create a recipe to create the basic configuration file.

The const $ = require('jquery'); ... is not necessary if you use the .autoProvidejQuery() call in encore. But this could collide with other code as I had problems where I had to load code for sonata admin and other frontend code and autoProvidejQuery messed things up, can't remember the details though. If you consider rewriting the JS i'd rather use TypeScript than ES6, but that's personal preference. It would provide proper types for those who use TS and it's easy to compile to ES6.

Another thing which would be nice is having proper validation of the formtype options. So if you're missing or combining invalid options it should throw an exception, this would make it much friendlier to newer devs.

The autocomplete service could handle more things, like set the required user roles as type option and check against that when providing auto completion. A predefined routing which could be configured would also be nice. It would be possible to pass the form type in the request which would make custom controllers obsolete in a lot of cases, but for that it should be done secure as it could lead to a lot of security problems. i.E. only allow using the route when roles are set.

@tacman
Copy link

tacman commented Jul 20, 2019

Yes to moderizing, Symfony 5 will be out in 4 months, so moderizing toward Symfony 4.4 without deprecations will allow the bundle to work with Symfony 5.

I've put together a project with some (incomplete) documentation on how to quickly deploy the bundle. My idea was to build out a bunch of working examples (Ajax calls, Add new entries, etc.) It tries to use the most up-to-date components (bootstrap4, webpack, latest Symfony, etc.)

https://select2-demo.herokuapp.com/

Source and documentation:

https://github.com/survos/select2entity-bundle-demo

I built the project originally to isolate a bug with selecting multiple tags with select2 itself. So the demo project locks the select2 library at 4.0.5.

@tetranz
Copy link
Owner Author

tetranz commented Jul 28, 2019

Thanks for the comments. I agree that it would be nice to have it ready for Symfony 5. I'll look through the deprecations and see if I can figure out what needs to change and probably start a version 3 branch. I'm open to pull requests of course.

Your demo looks good @tacman

@Azraelir
Copy link

Azraelir commented Sep 8, 2019

@tetranz

can you add this for next version?

#68 (comment)

#140

@DubbleClick
Copy link
Contributor

DubbleClick commented Sep 26, 2019

Given that the PR for a Symfony 4/5 version was added, maybe we should make a recipe for the bundle. @tetranz will you create a repository?
Edit: https://github.com/DubbleClick/tetranzselect2entity_recipe
Do you know if it's possible to add the form_theme automatically in twig.yaml?

@tetranz
Copy link
Owner Author

tetranz commented Sep 26, 2019

(Sorry, I replied first as the wrong user).
Yes a recipe sounds like a good idea. I must admit that I haven't taken much notice of Flex. Let me do some quick reading first. It's going to be a busy weekend for me so it might be early next week.

@DubbleClick
Copy link
Contributor

I've so far only managed to automatically set up the config yaml and to automatically activate/deactivate the bundle. I'm not sure how possible linking the javascript or the form_theme is. Documentation to flex recipes is... uh, sparse, in my opinion.

@tetranz
Copy link
Owner Author

tetranz commented Oct 5, 2019

Hi @DubbleClick Sorry I took so long. I'm taking a look at Flex recipes etc now.

I've created this. https://github.com/tetranz/select2entity-bundle_recipe
I think that what you wanted. Are you going to do a PR against that?
Then I guess I need to fork this https://github.com/symfony/recipes-contrib and make a PR.

I don't have any experience with Flex with or without Twig but, just picking another at random, it seems like we should be able to do the Twig config.

https://github.com/symfony/recipes-contrib/blob/master/friendsofsymfony/ckeditor-bundle/2.0/config/packages/fos_ckeditor.yaml

If we can't get the form theme to be automatically configured then maybe it's not worth the effort.

@DubbleClick
Copy link
Contributor

DubbleClick commented Oct 6, 2019

That's an interesting find, but wouldn't that just copy the recipe there? Can you have multiple twig: keys? If that works then might as well simply add that. Can't fork or create PR's for your repository by the way because it's empty. Maybe you can just copy the recipe I created and go from there.

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

No branches or pull requests

5 participants