Skip to content

[4.2] Fix empty default_value check in fields plugin#38426

Merged
roland-d merged 1 commit intojoomla:4.2-devfrom
HLeithner:fix/menuitem
Aug 9, 2022
Merged

[4.2] Fix empty default_value check in fields plugin#38426
roland-d merged 1 commit intojoomla:4.2-devfrom
HLeithner:fix/menuitem

Conversation

@HLeithner
Copy link
Copy Markdown
Member

@HLeithner HLeithner commented Aug 8, 2022

Pull Request for Issue introduced in #18032 .

Summary of Changes

  • Don't add a default_value attribute if value is NULL. This crashes GroupedlistField and maybe other fields.
  • Hide default value field custom fields for the menuitem

Testing Instructions

Test custom fields, especially menuitem

Actual result BEFORE applying this Pull Request

see #38244 (comment)

Expected result AFTER applying this Pull Request

works

@obuisard
Copy link
Copy Markdown
Contributor

obuisard commented Aug 8, 2022

I have tested this item ✅ successfully on 51ea6ce

Works as expected with the menuitem field.

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

@jwaisner
Copy link
Copy Markdown
Member

jwaisner commented Aug 8, 2022

I have tested this item ✅ successfully on 51ea6ce

Works with menuitem and other custom fields.

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

@jwaisner
Copy link
Copy Markdown
Member

jwaisner commented Aug 8, 2022

RTC

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

@joomla-cms-bot joomla-cms-bot added the RTC This Pull Request is Ready To Commit label Aug 8, 2022
@Quy Quy added the PR-4.2-dev label Aug 8, 2022
@laoneo
Copy link
Copy Markdown
Member

laoneo commented Aug 9, 2022

Looks good to me, just wondering why this custom field can't have a default value.

@roland-d roland-d merged commit 19a2b79 into joomla:4.2-dev Aug 9, 2022
@joomla-cms-bot joomla-cms-bot removed the RTC This Pull Request is Ready To Commit label Aug 9, 2022
@roland-d
Copy link
Copy Markdown
Contributor

roland-d commented Aug 9, 2022

Thank you

@roland-d roland-d added this to the Joomla! 4.2.0 milestone Aug 9, 2022
@HLeithner HLeithner deleted the fix/menuitem branch August 9, 2022 07:30
@HLeithner
Copy link
Copy Markdown
Member Author

HLeithner commented Aug 9, 2022

Looks good to me, just wondering why this custom field can't have a default value.

I was wondering too, I only had a quick look while fixing it. It happens here:
https://github.com/joomla/joomla-cms/blob/4.2-dev/libraries/src/Form/Field/GroupedlistField.php#L134
but to me that doesn't make any sense on the first glance because I would expect that the default_value is an attribute and not an element.

But since I found the issue in the default_value === '' check I fixed this one since maybe other fields have this problem too.

If someone wants to have a look at GroupedlistField he or she is welcome to do it.

roland-d added a commit that referenced this pull request Aug 14, 2022
@roland-d
Revert "Fix empty default_value check in fields plugin (#38426)"
7ef0a62 
This reverts commit 19a2b79.
@roland-d roland-d mentioned this pull request Aug 14, 2022
Merged
brianteeman added a commit to brianteeman/joomla-cms that referenced this pull request Nov 11, 2022
@brianteeman
[4.2] Menu Item Plugin
1a3a141 
This is a combination of joomla#38244 and joomla#38426
@brianteeman brianteeman mentioned this pull request Nov 11, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@laoneo laoneo laoneo approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

Joomla! 4.2.0

Development

Successfully merging this pull request may close these issues.

8 participants

@HLeithner @obuisard @jwaisner @laoneo @roland-d @Quy @richard67 @joomla-cms-bot