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
herrvigg opened this issue Jun 21, 2018 · 16 comments
Closed

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

herrvigg opened this issue Jun 21, 2018 · 16 comments
Labels
legacy issue Legacy issue imported from original repo

Comments

@herrvigg
Copy link
Collaborator

Issue by beheist
Saturday Jan 10, 2015 at 08:10 GMT
Originally opened as qTranslate-Team/qtranslate-x#1


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

@herrvigg herrvigg added the legacy issue Legacy issue imported from original repo label Jun 21, 2018
@herrvigg
Copy link
Collaborator Author

Comment by johnclause
Saturday Jan 10, 2015 at 19:19 GMT


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.

@herrvigg
Copy link
Collaborator Author

Comment by LC43
Saturday Jan 10, 2015 at 19:21 GMT


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 👍

@herrvigg
Copy link
Collaborator Author

Comment by LC43
Saturday Jan 10, 2015 at 19:40 GMT


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.

@herrvigg
Copy link
Collaborator Author

Comment by LC43
Saturday Jan 10, 2015 at 19:46 GMT


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...

@herrvigg
Copy link
Collaborator Author

Comment by johnclause
Saturday Jan 10, 2015 at 20:16 GMT


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.

@herrvigg
Copy link
Collaborator Author

Comment by LC43
Saturday Jan 10, 2015 at 20:20 GMT


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() ?

@herrvigg
Copy link
Collaborator Author

Comment by johnclause
Saturday Jan 10, 2015 at 20:29 GMT


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?

@herrvigg
Copy link
Collaborator Author

Comment by LC43
Saturday Jan 10, 2015 at 20:33 GMT


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.

@herrvigg
Copy link
Collaborator Author

Comment by LC43
Monday Jan 12, 2015 at 15:30 GMT


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.

@herrvigg
Copy link
Collaborator Author

Comment by johnclause
Monday Jan 12, 2015 at 17:13 GMT


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.

@herrvigg
Copy link
Collaborator Author

Comment by johnclause
Monday Jan 12, 2015 at 17:19 GMT


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.

@herrvigg
Copy link
Collaborator Author

Comment by LC43
Monday Jan 12, 2015 at 17:46 GMT


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
qTranslate-Team/qtranslate-x#1 (comment)
.

@herrvigg
Copy link
Collaborator Author

Comment by LC43
Monday Jan 12, 2015 at 17:47 GMT


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
qTranslate-Team/qtranslate-x#1 (comment)
.

@herrvigg
Copy link
Collaborator Author

Comment by johnclause
Monday Jan 12, 2015 at 17:52 GMT


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

@herrvigg
Copy link
Collaborator Author

Comment by qtranslateteam
Wednesday Jan 14, 2015 at 02:30 GMT


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:

qTranslate-Team/qtranslate-x#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 qTranslate-Team/qtranslate-x#1.


Reply to this email directly or view it on GitHub
qTranslate-Team/qtranslate-x#1 (comment)
.

@herrvigg
Copy link
Collaborator Author

Comment by LC43
Wednesday Jan 14, 2015 at 21:35 GMT


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
legacy issue Legacy issue imported from original repo
Projects
None yet
Development

No branches or pull requests

1 participant