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

[4.0] add fields items to contact menu #26540

Merged
merged 20 commits into from Oct 19, 2019
Merged

[4.0] add fields items to contact menu #26540

merged 20 commits into from Oct 19, 2019

Conversation

alikon
Copy link
Contributor

@alikon alikon commented Oct 10, 2019

Pull Request for replace of #23366 .

Summary of Changes

Add menu items for fields and field groups below the "Contact" menu item in the admin menu.

On update, use script.php to add these new menu items so that lft and rgt are set correctly in the (nested) menu table.

Testing Instructions

If you have both MySQL and PostgreSQL, please test on both if possible.

Test 1: New installation

  1. Apply the patch on a clean 4.0-dev branch using git or merging manually, or apply it on an installation of current 4.0-dev using patchtester.
  2. If used patchtester on an existing installation in step 1, delete file configuration.php and delete all Joomla database tables in PhpMyAdmin or PhpPgAdmin (depending on your database type).
  3. Do a new installation, login to backend and check the available items below the "Contact" item in the admin menu.

Result: See section "Expected result" below.

Test 2: Update

  1. Update a 3.9.12 or staging to current 4.0-dev plus the patch of this PR applied by using the "Upload & Update" tab of the Joomla Update Component and the update zip container previously downloaded from following URL: https://test5.richard-fath.de/Joomla_4.0.0-beta1-dev-Development-Update_Package_2019-10-19_pr-26540.zip. ATTENTION: When working on a git repository you should know how to clean up your branch afte this test.
  2. Login to backend and check the available items below the "Contact" item in the admin menu.

Result: See section "Expected result" below.

Expected result

you have:

  • Fields
  • Fieldsgroup

below Contact menu item
Screenshot from 2019-10-10 19-39-04

Actual result

You don't have these 2 menu items below Contact component

Additional comments

We need to review how we deal with changes of content in nested tables when updating because issues with that are is quite hard to fix with update sql scripts. (personal consideration)

@alikon alikon marked this pull request as ready for review October 10, 2019 17:42
@alikon alikon changed the title [WIP][4.0] add fields items to contact menu [4.0] add fields items to contact menu Oct 10, 2019
@richard67
Copy link
Member

richard67 commented Oct 10, 2019

I think it will need some update sql script for that, too. If there is some 4.0-*.sql already existing which deals with these menu items, this one can be modified before we are in beta, otherwise or if you are not sure, make a new one. I can help with that on weekend. Edit: Just after I typed that I did read statement in the description about update script not needed.

@alikon
Copy link
Contributor Author

alikon commented Oct 10, 2019

we need to deeply review how we provide updates ---- if we are talking about sql
maybe i'm a dumb
...but it has been quite hard 4 me to to deliver this PR ....
update path without a stored procedure could be full of issues.... when you are talking about nested set tables, then cannot be a matter for easy/silly update .... but... maybe i'm too silly 😄

@brianteeman
Copy link
Contributor

That's why I asked someone else to do it :)

@richard67
Copy link
Member

Hmm, thinking more about it: If it is really ok in 3.9 and not ok in 4.0, maybe there is already some 4.0.0-*.sql update script which removes those menu items? If this is the case, then this update script has to be changed. I can check that on weekend, but tonight and tomorrow night I won't have time.

@richard67
Copy link
Member

Well, I just checked: Seems there is no 4.0 update sql deleting those menu items, so everything should be ok, no update sql needed.

@alikon
Copy link
Contributor Author

alikon commented Oct 10, 2019

let me clarify...
no better
let me try to explain myself as better as I can ..
when nested set tables are in play..
imho there are no static script that can save your ass

@richard67
Copy link
Member

Correct.

@alikon
Copy link
Contributor Author

alikon commented Oct 10, 2019

you cannot guess what are the different lft,rgt values for all installation--- so
the update path should be at least careafully considered those different situation
my answer ----maybe we should consider a stored procedure......or
i'm open to feedaback

@richard67
Copy link
Member

but shouldn't the "rebuild" button do that after an update?

@alikon
Copy link
Contributor Author

alikon commented Oct 10, 2019

are you talking about menus table only ?
i'm talking about the whole update process---- i.e assets table .... etc
better ... i'm talking about an update path where nested set table are deeply involved

@richard67
Copy link
Member

Well I did not wanna speak against it, I only asked for my better understanding.

@alikon
Copy link
Contributor Author

alikon commented Oct 10, 2019

i hope i've at least instilled some grams of curiosity 😃

@brianteeman
Copy link
Contributor

its also why I would not have had any 4.0sql update files at all and just done them all at once when we get to beta. just creates a lot of useless files that will be deleted

@Quy
Copy link
Contributor

Quy commented Oct 11, 2019

Add a separator between Categories and Fields.

@alikon
Copy link
Contributor Author

alikon commented Oct 11, 2019

after a more proof review of 3.x we still need an update sql .... i was wrong

@SniperSister
Copy link
Contributor

@richard67
Copy link
Member

@SniperSister @alikon and me know about this. I've made a PR to his branch to fix it, he will merge as soon as lunch finished, I think.

Fix postgresql/joomla.sql
@richard67
Copy link
Member

@SniperSister Tests are passing here now.

@richard67
Copy link
Member

I have tested this item ✅ successfully on 661f93b

Test 1 new installation - MySQL 5.7: OK.
Test 1 new installation - PostgreSQL 10.10: OK.
Test 2 update - MySQL 5.7: OK.
Test 2 update - PostgreSQL 10.10: OK.


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

@richard67
Copy link
Member

@brianteeman Could you test this, too? I've helped with updating testing instructions and making a zip package for an update test because Nicola is a bit busy with preparing for JoomlaDay Italy, so see the updated instructions for details.

Co-Authored-By: Quy <quy@fluxbb.org>
@infograf768
Copy link
Member

infograf768 commented Oct 14, 2019

Installed on a 3.9.12

I get
Screen Shot 2019-10-14 at 12 18 20

and
Notice: Trying to get property 'version' of non-object in /Applications/MAMP/htdocs/trunkgitnew/administrator/components/com_joomlaupdate/Model/UpdateModel.php on line 1357

Contacts have no fields
Screen Shot 2019-10-14 at 12 21 32
All dashboards are empty. No way to access to database page.

No way to save the administrator menu to use Alternate preset in order to access database page
Save failed with the following error: Column 'publish_down' cannot be null

used directly the link
administrator/index.php?option=com_installer&view=database

2 problems for database
Used Update structure button.
1 problem remains and the button is no more displayed. Menus disappear.

@richard67
Copy link
Member

@infograf768 The first message about template not available is normal after the update but should disappear and not come again after having liogged in to backend for the first time and then having navigated 1 time withint the backend.

The 2nd message should be fixed when my database null date works will be finished. It normally does not appear on a pre 8.0 MySQL with normal configuration regarding session parameters and server variables, e.g. for strict mode. Did you test this on MySQL 8?

The SQL error and empty dashboards ... maybe I've made a mistake when packing the update zip package which is linked in the testing instructions for test 2? I have to check that tonight after work (German time).

@infograf768
Copy link
Member

Did you test this on MySQL 8?

Nope.

@nadjak77
Copy link
Contributor

I have tested this item 🔴 unsuccessfully on 4f755bc

Deleted configuration.php and databasetables
new installation
applied patch
deleted browsercache and reload site
the two menue items for "fields" and "fields groups" do not appear


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

@richard67
Copy link
Member

@nadjak77 Wrong order of processing. It has to be

  1. Apply patch on a 4.0-dev or alpha-12. This applies the changes to joomla.sql.
  2. Remove configuration.php and clear database.
  3. Run new installation. This will use the changed joomla.sql.
  4. Check backend menu.

You have done the new installation and then applied the patch but then have not done new installation again, so your database did not include the changes here.

@richard67
Copy link
Member

I've just updated the update package for Test 2 to latest nightly build of today.

Testers please follow exactly the testing instructions given in the description of this PR.

@richard67
Copy link
Member

@nadjak77 Please change back your testing result to "not tested" in the issue tracker. The test failed because you did not follow exactly the testing instructions. And if you have enough time, please test again in the right way, that would be very appreciated. Anyway thanks for testing, even if wrong now, because we need volunteers who help with testing.

@nadjak77
Copy link
Contributor

I have tested this item ✅ successfully on 4f755bc

thank you @richard67
I tried it again:
apply patch
delete configuration.php and database
reinstall joomla
the new items appear under contact


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

@tecpromotion
Copy link
Contributor

I have tested this item ✅ successfully on 4f755bc


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

1 similar comment
@uthorat
Copy link

uthorat commented Oct 19, 2019

I have tested this item ✅ successfully on 4f755bc


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

@Quy
Copy link
Contributor

Quy commented Oct 19, 2019

RTC


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

@joomla-cms-bot joomla-cms-bot added the RTC This Pull Request is Ready To Commit label Oct 19, 2019
@wilsonge wilsonge merged commit b5351d2 into joomla:4.0-dev Oct 19, 2019
@wilsonge
Copy link
Contributor

Thanks!

@joomla-cms-bot joomla-cms-bot removed the RTC This Pull Request is Ready To Commit label Oct 19, 2019
@wilsonge wilsonge added this to the Joomla 4.0 milestone Oct 19, 2019
@astridx
Copy link
Contributor

astridx commented Oct 20, 2019

Thank you for making the custom fields usable again.
Now I'm wondering: how should third party developers integrate custom fields in Joomla 4? Is it recommended that you also change the table #__menu, or should you continue to use a sub menu?

@alikon alikon deleted the patch-122 branch October 21, 2019 07:16
@alikon
Copy link
Contributor Author

alikon commented Nov 9, 2019

Now I'm wondering: how should third party developers integrate custom fields in Joomla 4? Is it recommended that you also change the table #__menu, or should you continue to use a sub menu?

imo is matter of personal taste / consistency

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.

None yet