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

qtranslate-x-2.8 + qtranslate-slugs is broken #1

Closed
beheist opened this issue Jan 10, 2015 · 16 comments
Closed

qtranslate-x-2.8 + qtranslate-slugs is broken #1

beheist opened this issue Jan 10, 2015 · 16 comments

Comments

@beheist
Copy link

beheist commented Jan 10, 2015

Good morning John,

I just upgraded my blog to qTranslate-X 2.8. After doing that, none of my posts were reachable anymore - everything resulted in 404s. When I disabled qTranslate-slugs, my posts were reachable again, so there seems something in qTranslate-X 2.8 that breaks if slugs is also used.
This issue has made me rethink the current approach with qTranslate-X and qTranslate slugs. I think we're running into a fundamental issue here - as long as the two plugins are separate and are thus not always tested together, issues like these will keep cropping up. Of course, it'd be possible to try and fix slugs everytime qTX gets upgraded (or the other way round), but that would lead to a lot of uncertainty and frustration for users because it's not ensured that a working combination is installed.
So I'd like to discuss whether you think it makes sense to include the slugs functionality in qTranslate-X itself. If you agree (and also if @LC43 doesn't see a problem with that since it's his code) I would volunteer to include that functionality. What do you think?

Best regards,
Bastian

@johnclause
Copy link
Member

Hello Bastian,

I normally test a few plugins that were already reported having compatibility problems, but -slug was not on my list yet, since I did not see a release which works so far. Are you testing your own patch? In general, I think it is better to have it separate for the sake of spreading the support load, unless @LC43 does not mind to watch qTranslate-X forum and pick up the issues related to -slug. How do we include @LC43 in the discussion, will he get this message automatically when he is mentioned here? How can I test what you are testing? I am new to GitHub and learning now how it works, any tips would be helpful. Thanks a lot.

@LC43
Copy link

LC43 commented Jan 10, 2015

hi everyone, please go ahead. I was looking at your code to see what did change.

I'll try to keep our own code updated, but i can't promise. I have my hands full fixing our bug and my day job.

ps: yes, i get included if you meantion me 👍

@LC43
Copy link

LC43 commented Jan 10, 2015

one of the things that broke was that you changed the vars QT_* to QTX_* .

Changing QT_URL_PATH to QTX_URL_PATH was enough to break our plugin.

you also removed qtrans_useTermLib(), and i think i saw that you change the number of arguments of some other function.

@LC43
Copy link

LC43 commented Jan 10, 2015

and its my fault that i didn't released @beheist code that made our plugin compatible with yours. :/ every time i'm about to push it to wordpress, a bug creaps in...

@johnclause
Copy link
Member

Oh my ... I hope I am done with the renamings, it stabilized now, although, we should think how to design proper hooks, so that neither you nor other people would need to use qtrans* functions directly. I have to see how your plugin works.

@LC43
Copy link

LC43 commented Jan 10, 2015

i'm merging some of my working branches so, if all goes well, i'll push to wordpress our latest version that is also compatible with yours.

on a side note, why did you delete qtrans_useTermLib() ?

@johnclause
Copy link
Member

I never had qtrans_useTermLib, but it is available under qtranxf_useTermLib name. The comment you saw, apparently in qtranslate_compatibility.php, is wrong, and that file is not in use yet. I tried to put it in but it generates fatal errors under some circumstances.

I am thinking about adding a boolean option to include this file on a user conscious request, since I got a number of requests for some of those qtrans* functions to exist. Do you think, such an option would be a good idea?

@LC43
Copy link

LC43 commented Jan 10, 2015

ah, that makes sense now :D

well, im using your plugin with
require_once(dirname(FILE)."/qtranslate_compatibility.php"); to test on some other branches that didn't had the code from Bastian.

i also added qtrans_getSortedLanguages() to the list.

@LC43
Copy link

LC43 commented Jan 12, 2015

by removing wphacks, this breaks our plugin in the taxonomies. we were using qtrans_modifyTermForm() .... which then called qtrans_insertTermInput and qtrans_insertTermInput2. would you consider stopping breaking the api, by not changing/removing public functions? its really hard to stay compatible if you keep up with this.

@johnclause
Copy link
Member

Hi Pedro, I am really sorry, I get frustrated with other people changes sometimes too :) It is a period of birth and quick grow. It will stabilize after a while.

On this particular case, in fact, I did not remove it, it was not there already to begin with in zTranslate, which I started with. And it is not needed for qTranslate-X. That one was the source of main problem of original qTranslate and I felt very relieved to see it go away in zTranslate. The name of that piece was good, "wphacks", and "WP hacks" is exactly what we should not do to survive WP updates without changes. Did you know that on last WP update to 4.1 all other forks of qTranslate got broken and had to do some adjustments, except qTranslate-X, which kept working just as it was?

If you use those functions, I do not understand how did you get away so far without them? They could have been defined in other active plugin during the testing - yeah, this is how it was probably.

Anyway, please, add back those on your plugin for now with If(function_exists) wrapper and it will be good. qTranslate-X will never need them anymore.

I think it would be eventually a good idea to adjust your slug management to the same logic as it was done with title fields and apply it to "slug" field. We now need one "slug" field only and Language Switching Buttons will put value needed per language there. I understand that this is a much bigger change for -slug, and we do not need to do it right away. I believe this would belong to my plate, and I can get busy with this after the things are stabilized.

@johnclause
Copy link
Member

BTW, at some point, we will probably need to bring the code to a contemporary standard with PHP classes instead of a bunch of global variables. We will need to figure out who is using direct calls to internal functions, and advise them to use hooks instead, and we will need to provide a reasonable set of hooks. This will break the -slug one more time, so we will need to synchronize that change, but I do not plan to do it any soon, if ever in fact.

@LC43
Copy link

LC43 commented Jan 12, 2015

Hi john!

you are right, sorry for venting out!

On 12 January 2015 at 17:13, johnclause notifications@github.com wrote:

Hi Pedro, I am really sorry, I get frustrated with other people changes
sometimes too :) It is a period of birth and quick grow. It will stabilize
after a while.

On this particular case, in fact, I did not remove it, it was not there
already to begin with in zTranslate, which I started with. And it is not
needed for qTranslate-X. That one was the source of main problem of
original qTranslate and I felt very relieved to see it go away in
zTranslate. The name of that piece was good, "wphacks", and "WP hacks" is
exactly what we should not do to survive WP updates without changes. Did
you know that on last WP update to 4.1 all other forks of qTranslate got
broken and had to do some adjustments, except qTranslate-X, which kept
working just as it was?

Great news!

If you use those functions, I do not understand how did you get away so
far without them? They could have been defined in other active plugin
during the testing - yeah, this is how it was probably.

Indeed, since it stopped working at this release i checked your master
branch and, since i couldn't find it, i just assumed that it got removed...
my bad...

Anyway, please, add back those on your plugin for now with
If(function_exists) wrapper and it will be good. qTranslate-X will never
need them anymore.

i've already did some changes to the original functions, i might as well
write them into our own code.

I think it would be eventually a good idea to adjust your slug management
to the same logic as it was done with title fields and apply it to "slug"
field. We now need one "slug" field only and Language Switching Buttons
will put value needed per language there. I understand that this is a much
bigger change for -slug, and we do not need to do it right away. I believe
this would belong to my plate, and I can get busy with this after the
things are stabilized.

Cool! looking forward!


Reply to this email directly or view it on GitHub
#1 (comment)
.

@LC43
Copy link

LC43 commented Jan 12, 2015

More great news! maybe do a couple of versions with the deprecated
functions still in the code to give everyone time to adjust to the new API.

On 12 January 2015 at 17:19, johnclause notifications@github.com wrote:

BTW, at some point, we will probably need to bring the code to a
contemporary standard with PHP classes instead of a bunch of global
variables. We will need to figure out who is using direct calls to internal
functions, and advise them to use hooks instead, and we will need to
provide a reasonable set of hooks. This will break the -slug one more time,
so we will need to synchronize that change, but I do not plan to do it any
soon, if ever in fact.


Reply to this email directly or view it on GitHub
#1 (comment)
.

@johnclause
Copy link
Member

Yeah, it is a good idea. This is how we will do it, when time comes 👍

@qtranslateteam
Copy link

First, thanks to all for participating, you have been and are of real help.
Please, re-open this issue if you think we are not done here, are we?

I submitted a new issue, for somebody who wishes to exercise the
troubleshooting skills:

#5

How would I normally invite other people to look at the issue?

On Tue, Jan 13, 2015 at 5:59 PM, johnclause notifications@github.com
wrote:

Closed #1 #1.


Reply to this email directly or view it on GitHub
#1 (comment)
.

@LC43
Copy link

LC43 commented Jan 14, 2015

well, either mention that person directly, or if you don't have anyone in particular in your mind, opening the issue will notify everyone that is watching.

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

4 participants