Skip to content

Conversation

@ryanmitchell
Copy link
Contributor

@ryanmitchell ryanmitchell commented Sep 5, 2022

This goes some of the way to resolving issue #43 in that it preserves the order, however if we're using incrementing IDs then ordering will fail once IDs hit double digits as it will treat it as string ordering rather than numeric ordering (so 1, 10, 2, 3 etc).

I'm not sure how to resolve this in the JSON field with getting into specific code for specific database types.

The best option might be to make an order column in the database for handling this?


Edit by Jason:

@ryanmitchell
Copy link
Contributor Author

ryanmitchell commented Sep 5, 2022

Update: I've tapped into laravel core builder methods to cast the order as a real field which seems to do the trick.

@enespolat24
Copy link
Contributor

image

Hi , i tried this pr again and i have time format issue while reordering , is it something to do with my mysql configuration ?

@ryanmitchell
Copy link
Contributor Author

ryanmitchell commented Sep 16, 2022 via email

@enespolat24
Copy link
Contributor

i'll try to supply a sample tonight but i temporarily eleminated the problem like this in the CollectionRepository
image

@ryanmitchell
Copy link
Contributor Author

ryanmitchell commented Sep 16, 2022 via email

@ryanmitchell
Copy link
Contributor Author

@enespolat24 can you create a sample repository so I can look into this further?

@enespolat24
Copy link
Contributor

enespolat24 commented Sep 21, 2022

hi , sorry for the latency
https://github.com/enespolat24/example-case

@ryanmitchell
Copy link
Contributor Author

@jasonvarga this is good to merge from our testing and the feedback from others

@enespolat24
Copy link
Contributor

@ryanmitchell I tested this with one of my live projects. We use mariaDb instead of mysql. Can we use DOUBLE PRECISION instead of REAL since mariadb doesn't support REALdatatype ?

@ryanmitchell
Copy link
Contributor Author

ryanmitchell commented Sep 28, 2022 via email

@enespolat24
Copy link
Contributor

I checked their docs. they all support DOUBLE PRECISION but in the Postgres there's a storage size difference. REAL uses 4 byte but DOUBLE PRECISION uses 8 byte

@ryanmitchell
Copy link
Contributor Author

ryanmitchell commented Sep 28, 2022 via email

@ryanmitchell
Copy link
Contributor Author

@jasonvarga would be great to get this merged as ordering isn't working in the CP at the moment.

@jasonvarga
Copy link
Member

@enespolat24 Does this PR fix your issue? Last you said on #43 was that you're still having issues.

@jasonvarga
Copy link
Member

@ryanmitchell What's the reason for using the orderByRaw and CAST stuff? Is it because you don't want to add an order column?

I'm using MariaDB 10.5.8 and it doesn't seem to like that syntax. I get the following error when I visit the reorder entries screen in the CP.

SQLSTATE[42000]: Syntax error or access violation: 1064 You have an error in your SQL syntax; check the manual that corresponds to your MariaDB server version for the right syntax to use near 'PRECISION) ASC' at line 1
select
  *
from
  `entries`
where
  `collection` = 'more_pages'
order by
  CAST(
    json_unquote(json_extract(`data`, '$."order"')) AS DOUBLE PRECISION
  ) ASC
limit
  50 offset 0

@enespolat24
Copy link
Contributor

@jasonvarga works like a charm with mysql but i tried this with our Mariadb 10.6.3 and got same error as you do. the walk around i did was changing DOUBLE PRECISION as DOUBLE. Still not optimum but does the job. with this temporary solution i need to click save button more than once

@ryanmitchell
Copy link
Contributor Author

ryanmitchell commented Oct 27, 2022 via email

@jasonvarga
Copy link
Member

Two things:

  1. Why are you calling the column sort_order instead of just order?
  2. How can we get that migration going? It's just a stub, how are people supposed to use it once they upgrade?

@ryanmitchell
Copy link
Contributor Author

Order is a reserved word in some db engines. I’ve always avoided using it as a result.

Yeah the migrations annoyed me too - I don’t like the approach of publishing then every time but others contributing to the big PR wanted it so I gave in.

To me it should automatically publish but I think we are stuck now?

@jasonvarga
Copy link
Member

Order is a reserved word in some db engines. I’ve always avoided using it as a result.

I would think as long as its quoted appropriately it'll be fine with it being a column name.

ie. select `title`,`order` from `entries` as opposed to just select title,order from entries
Laravel adds the quotes.

As for publishing the migrations.... even if you publish the migrations again, it publishes all of them, not just new ones. 🤔

@ryanmitchell
Copy link
Contributor Author

Ok I’ll change it over later on.

Yeah I should have really pushed back on the migrations, I knew it wasn’t the right approach - sorry about that.

do we need to do something like the upgrade to logic in core that you use for adding permissions etc?

@jasonvarga
Copy link
Member

Thanks. We'll figure out the migration side of it from here. Leave it with us.

Much appreciated!

@ryanmitchell
Copy link
Contributor Author

Sorry that ive left you with a mess to clean up!

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.

Imported Navs lose their order Reorder collection does not working

3 participants