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

Change type of field "value" in table #_fields_values from text to mediumtext #42606

Merged
merged 2 commits into from
Jan 26, 2024

Conversation

sergeytolkachyov
Copy link
Contributor

@sergeytolkachyov sergeytolkachyov commented Jan 4, 2024

Change type of field "value" in table #_fields_values from text to mediumtext for MySQL ONLY. PostgreSQL have only text field type with unlimited length. Pull Request for Issue #36065 and remake a PR #42605 for Joomla 5.1.0

Testing instructions

  1. Go to bottom of this PR and click Show all checks in All checks have passed section
  2. Click on Details in download line
    image
  3. You will go to https://artifacts.joomla.org/drone/joomla/joomla-cms/5.1-dev/42606/downloads/72696/
  4. Download Full installation package and update package of Joomla 5.1 + this PR
    image
  5. Make a clean install from Joomla_5.1.0-alpha3-dev+pr.42606-Development-Full_Package.zip
  6. Go to PhpMyAdmin, select database you used for clean install and go to the #__field_values table. Then go to structure section and check that type of value field is mediumtext.
    image
  7. Clean your database and test domain folder and install a Joomla 5.0.1
  8. Check value field type in PHP My Admin in #__field_values table. It is a text type now.
    image
  9. Then go to admin panel / System / Update / Joomla . Press the Upload and Update button
    image
  10. Upload the Joomla_5.1.0-alpha3-dev+pr.42606-Development-Update_Package and update
    image
  11. Then go to PhpMyAdmin again and check the #__field_values table - type of field value. It could be a mediumtext
    image

…diumtext for MySQL ONLY. PostgreSQL have only text field type with unlimited length. Pull Request for Issue #36065 and remake a PR #42605 for Joomla 5.1.0
@richard67
Copy link
Member

@sergeytolkachyov By review your PR looks good to me. But it would be good if you could provide some testing instructions. When you create a pull request, there are already headings for testing instructions and so on in the description. This is intended to be completed by the author, not to be removed completely. Possible testing instructions could be to make a new installation and check the type of the column after that, and to make an update with the package ur custom URL created by drone and check the type of the column after that. Or in this simple case maybe only "Code review". But there should be some testing instructions.

@richard67
Copy link
Member

@sergeytolkachyov You reacted with a thumbs up to my previous comment, but you haven't added any testing instructions to the description of this PR yet. Could you do that? Thanks in advance.

@sergeytolkachyov
Copy link
Contributor Author

@sergeytolkachyov You reacted with a thumbs up to my previous comment, but you haven't added any testing instructions to the description of this PR yet. Could you do that? Thanks in advance.

Yes, I'll do that

@bembelimen
Copy link
Contributor

Just an off topic comment, if you working heavily with custom fields: you can improve the performance enormous when you change the item_id column to integer instead of varchar.

But for this change we need a small kind of migration, but I think worth to investigate (if you're up for another PR)

@sergeytolkachyov
Copy link
Contributor Author

@sergeytolkachyov You reacted with a thumbs up to my previous comment, but you haven't added any testing instructions to the description of this PR yet. Could you do that? Thanks in advance.

Done

@richard67
Copy link
Member

Done

Thanks.

@chmst
Copy link
Contributor

chmst commented Jan 5, 2024

Just an off topic comment, if you working heavily with custom fields: you can improve the performance enormous when you change the item_id column to integer instead of varchar.

But for this change we need a small kind of migration, but I think worth to investigate (if you're up for another PR)

@bembelimen It did that in my application with about 40k fields_values and it makes such an incredible huge difference. I would really appreciate and support a PR for that.

@sergeytolkachyov
Copy link
Contributor Author

@richard67 @bembelimen not merged?

@richard67
Copy link
Member

richard67 commented Jan 25, 2024

@sergeytolkachyov You should know that

  1. Every non trivial pull request needs 2 successful tests by human testers before it can be merged, and
  2. I am not a release manager so I won’t decide anything.

@alikon
Copy link
Contributor

alikon commented Jan 25, 2024

quite every pr needs 2 human test

@alikon
Copy link
Contributor

alikon commented Jan 25, 2024

I have tested this item ✅ successfully on a7badaa

code review


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/42606.

@richard67
Copy link
Member

quite every pr needs 2 human test

Yes, if it’s not just a typo in a comment, that’s what I mean with „non trivial“.

@sergeytolkachyov
Copy link
Contributor Author

Strangely, for some reason I remembered that 2 necessary tests had already been done.

@richard67
Copy link
Member

Strangely, for some reason I remembered that 2 necessary tests had already been done.

@sergeytolkachyov Where? Here in the PR I can not see any human test results except of the one from a few minutes ago.

@sergeytolkachyov
Copy link
Contributor Author

Strangely, for some reason I remembered that 2 necessary tests had already been done.

@sergeytolkachyov Where? Here in the PR I can not see any human test results except of the one from a few minutes ago.

I wrote to 3 humans for testing now. Thank you

@gug2
Copy link

gug2 commented Jan 25, 2024

I have tested this item ✅ successfully on a7badaa

Successfully tested


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/42606.

@richard67
Copy link
Member

RTC


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/42606.

@joomla-cms-bot joomla-cms-bot added the RTC This Pull Request is Ready To Commit label Jan 25, 2024
@Razzo1987 Razzo1987 merged commit 1b2b0c1 into joomla:5.1-dev Jan 26, 2024
3 checks passed
@joomla-cms-bot joomla-cms-bot removed the RTC This Pull Request is Ready To Commit label Jan 26, 2024
@Razzo1987
Copy link
Contributor

Thanks!

@Razzo1987 Razzo1987 added this to the Joomla! 5.1.0 milestone Jan 26, 2024
@sergeytolkachyov sergeytolkachyov deleted the 5.1-dev branch January 31, 2024 06:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

9 participants