search

distantnative / retour-for-kirby

Kirby CMS plugin to manage redirects and track 404s right from the Panel
https://distantnative.com/retour-for-kirby/
MIT License
135 stars 15 forks source link

Routes config gets wiped #229

Closed bvrp closed 2 years ago

bvrp commented 3 years ago

Describe the bug

When adding a new entry by clicking on the "Add as redirect" on an entry under the Failures tab, when saving the new entry it a pop up window appears saying "Row is not defined". After dismissing the dialog, the new entry initially appears under the "Routes" tab, but a refresh of the page shows that the entire routes db that the plug uses is emptied.

Update: Adding new routes via the Routes tab also results in wiping out the Routes DB, except no error is show.

Update#2: Looks like what I thought were routes being added was only on the Routes tab local cached page DOM. No routes are actually being permanently saved. When refreshing the page, all routes added disappear.

Update#3: Ok- so routes are being written to redirects.yml by the plugin, however, reloading the plugin panel page in the browser causes it to overwrite the redirects.yml file with an empty one. It does not overwrite the "retour/log.sqlite" file though.

Steps to reproduce

  1. Click on "Add as redirect" on one of the rows under the "Failures" tab.
  2. Add your new route in resulting dialog
  3. Save the new entry
  4. Pop up shows "Row is undefined"
  5. Click ok to dismiss dialog
  6. note that the new entry is shown in the routes tab.
  7. refresh browser page, or exit to kirby panel and go back to "Redirects"
  8. Note that all route entries are now gone/deleted.

Expected behavior

That my new route is added without deleting all entries.

Additional context

E.g. screenshots

Specifications

chrfickinger commented 3 years ago

Issue #221 shows similar behavior. Maybe the issue is based on the same bug.

bvrp commented 3 years ago

Yes, sure seems the same. From the looks of it, that thread you refer to was not resolved either, even though a "fix" was posted.

distantnative commented 3 years ago

I can confirm the "Row not defined" and routes wiping bugs are not related as it seems. I already found the "Row not defined" bug and fixed it.

On routes being wiped. The good news it, I just experienced it myself. So that's at least a chance of a lead :)

distantnative commented 3 years ago

I invested some time into config file reading/saving improvements and unit tests. I haven't run into the issue myself again since then. But not sure if I really caught it. Anyone feeling brave to test the develop branch whether it really solves the problem?

bvrp commented 3 years ago

I've installed your new version. Entries appear to be saved correctly. But there is also an "empty" route being saved somehow that shows up in the interface. When clicking on it to edit it- then clicking away to make the DLOG disappear without adding anything to the entry or saving it, an error "t.trim" is not a function appears.

An "empty" route entry should never appear in the Routes list for any reason. Preventing that should remove possibility of such an error.

bvrp commented 3 years ago

Another weird ui issue I am seeing during this- clicking away from the dialog (not by clicking the checkmark in upper right) apparently still saves the entry. (probably reason for "t.trim" error mentioned above). This should be handled as a "cancel" with no changes to be made. As it is currently, there is no way to "cancel" any changes made to the route DLOG.

Would be better to use proper ui conventions/process of "Save" "Cancel" etc.

distantnative commented 3 years ago

@bvrp using the core drawer component which doesn't support a cancel event. It's just open or close. So we have to live with that and treat it as direct manipulation on the dataset, not one that can be canceled

bvrp commented 3 years ago

Why even use a drawer to begin with. Non-cancellable drawer should never be the way to input data anywhere. Should be a pop-up really, no? Otherwise why even have the "checkmark" ok button at all?

chrfickinger commented 3 years ago

Just tested the develop branch. Seems placeholders, e.g. (:all) do not work anymore.

chrfickinger commented 3 years ago

The error is still present in the develop version. The redirects delete themselves automatically. I can not reproduce when and what exactly happens. They are just empty at some point without being worked on.

distantnative commented 3 years ago

I'm sorry guys, wish this wouldn't be so painful for you. I'll try to dig deeper.

bvrp commented 3 years ago

The error is still present in the develop version. The redirects delete themselves automatically. I can not reproduce when and what exactly happens. They are just empty at some point without being worked on.

Really? That's odd because it appears to be working fine now on my site. Odd indeed.

nilshoerrmann commented 3 years ago

I'm not sure if this is related to the issue described here, but I noticed that after updating Retour and clearing Kirby's media folder, all redirects were unset on next panel load.

nilshoerrmann commented 3 years ago

Anyone feeling brave to test the develop branch whether it really solves the problem?

Editing an existing entry just caused all my routes to disappear using the develop branch. Nothing suspicious in the console. The log itself is still intact.

johannschopplich commented 3 years ago

Running into the same issue myself. Does the v4 branch contain a fix for this issue already? I'm ready to test. 🙂

yatil commented 3 years ago

I am also affected by the issue – seems random, but I don’t know.

nilshoerrmann commented 3 years ago

I'm not sure if that's related but I noticed that I have two Retour databases in my logs folder, retour.sqlite and retour/log.sqlite. Is that correct?

distantnative commented 3 years ago

@johannschopplich aiming for the Easter break to finish v4

guidoferreyra commented 3 years ago

I’m also having the issue of the config file being wiped.

distantnative commented 3 years ago

I just released a first beta of Retour 4.0.0: https://github.com/distantnative/retour-for-kirby/releases/tag/4.0.0-beta.1

Since you are majorly affected by the flaws of v3, hopefully this will solve your problems. It would be great if some of you could try out this beta to see if this really solves the issues and to make sure we don't have new regressions.

Apologies for all the troubles and thanks for your patience and understanding!

yatil commented 3 years ago

@distantnative How do you want us to report bugs/observations on the 4.0.0 beta?

distantnative commented 3 years ago

@yatil best to just open new issues and mention that you're referring to the beta :)

chrfickinger commented 3 years ago

The error is still present in v.4.0.0. I can not reproduce when and what exactly happens, but at one point the redirects delete themselves automatically. They are just empty at some point without being worked on.

UPDATE: It seems the redirect.yml is still ok, but the list in the panel is empty.

distantnative commented 3 years ago

Sigh :(

It would be really good to have any info on whether an error appears in the console or what actions lead up to this. I'm really trying to kill that big but it seems really difficult with it seeming so random

guidoferreyra commented 3 years ago

@distantnative Since the beta 4.0 doesn’t works for me what do you recommend to get a stable experience? Go back to 3.0?

sebastiangreger commented 3 years ago

Hi; I just updated from 3.x to 4.0.0-beta.1 and while the redirect.yml was still intact, no routes were displayed in the panel.

After digging into the source code, it seems there is a typo in the release notes:

Rename your config file to site/config/redirects.yml

should actually be site/config/retour.yml. I renamed the YML file and now 4.0 works smoothly :slightly_smiling_face:

andreasfehn commented 3 years ago

Hi; I just updated from 3.x to 4.0.0-beta.1 and while the redirect.yml was still intact, no routes were displayed in the panel.

After digging into the source code, it seems there is a typo in the release notes:

Rename your config file to site/config/redirects.yml

should actually be site/config/retour.yml. I renamed the YML file and now 4.0 works smoothly 🙂

I also needed to change the first line ("routes" into "redirects:"). 4.0.0-beta.1 seems to work (for me) now. :) Thanks for your incredible work, @distantnative!

fvsch commented 3 years ago

Same problem of wiped redirects.yml here. Tried the 4.0.0-beta1 but it doesn't seem to support (:all) placeholders correctly:

Screenshot

I'll try to stay on 3.0.1 and restore my redirects.yml from Git for now. :)

derunterstrich commented 3 years ago

Hello,

I have the same problem, but I can recreate it relatively accurately. I create my routes and then copy the redirects.yml. If I wait ~20 minutes and press command + R, everything is gone. If I then restore my redirects.yml from the backup, everything is there again.

I installed it via composer (version 3.0.1). Kirby has the latest version

distantnative commented 2 years ago

This hopefully is solved by Retour 4.0 - first beta. Please help me test the beta, so that we can make sure that the actual release for sure doesn't suffer from the same problems as Retour 3 did.

Thanks everyone for your patience with this, I know it has been a pain point for many months.