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

Add guard-clause to clearDirty method #16467

Conversation

sebastian-marinescu
Copy link
Contributor

What does it do?

This makes sure the JS doesn't throw an error in the case that the items-object is an array, just like in #16404 for 3.x

Why is it needed?

When using ExtJS to create components of the xtype: 'radiogroup' to be used in CMPs inside a form,
this function gets called and tries to iterate over it's items array with a method that doesn't exist.

This PR isn't changing the way forms or the clearDirty-method are handled, it just assures no error is thrown when using radiogroup-components.

Related issue(s)/PR(s)

Resolves modmore/ClientConfig#202
Resolves modmore/ClientConfig#176
Resolves modmore/ClientConfig#143

@sebastian-marinescu
Copy link
Contributor Author

@opengeek @Mark-H

Hi guys, can we also tag/label this for the v2.8.6-milestone, please?

I'd appreciate it ❤️

@Ibochkarev Ibochkarev added this to the v2.8.6 milestone Sep 16, 2023
@Ibochkarev Ibochkarev added the pr/review-needed Pull request requires review and testing. label Sep 16, 2023
@sebastian-marinescu
Copy link
Contributor Author

Thank you very much @Ibochkarev

theboxer
theboxer previously approved these changes Jan 30, 2024
Copy link
Member

@theboxer theboxer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@rthrash rthrash added pr/ready-for-merging Pull request reviewed and tested and ready for merging. and removed pr/review-needed Pull request requires review and testing. labels Feb 8, 2024
Mark-H
Mark-H previously approved these changes Feb 10, 2024
@opengeek opengeek removed the pr/ready-for-merging Pull request reviewed and tested and ready for merging. label Feb 27, 2024
@opengeek
Copy link
Member

This change is currently breaking the grunt build with the following error:

Running "uglify:jsgrps" (uglify) task
JS_Parse_Error [SyntaxError]: Unexpected token: punc «.»
    at JS_Parse_Error.get (eval at <anonymous> (/Users/opengeek/www/revo-2.x/_build/templates/default/node_modules/uglify-js/tools/node.js:18:1), <anonymous>:71:23)
    at formatError (internal/util/inspect.js:1149:38)
    at formatRaw (internal/util/inspect.js:919:14)
    at formatValue (internal/util/inspect.js:774:10)
    at inspect (internal/util/inspect.js:319:10)
    at formatWithOptionsInternal (internal/util/inspect.js:1979:40)
    at formatWithOptions (internal/util/inspect.js:1861:10)
    at console.value (internal/console/constructor.js:328:14)
    at console.log (internal/console/constructor.js:364:61)
    at /Users/opengeek/www/revo-2.x/_build/templates/default/node_modules/grunt-contrib-uglify/tasks/uglify.js:144:17 {
  filename: 'util/utilities.js',
  line: 85,
  col: 22,
  pos: 2432
}

@rthrash rthrash added the bug The issue in the code or project, which should be addressed. label Feb 27, 2024
@opengeek
Copy link
Member

I'd really like to move forward on this. What needs to be done?

@opengeek opengeek added the blocked Active participation around the pull request or issue required. Consensus is not reached. label Mar 18, 2024
@smg6511
Copy link
Collaborator

smg6511 commented Mar 18, 2024

I'd really like to move forward on this. What needs to be done?

Hey Jason -- working on the build routine for 2.x to bring it in line with 3.x (will solve these problems that are cropping up) ... should be good in the next day or two.

opengeek added a commit that referenced this pull request Mar 21, 2024
### What does it do?
Replaces most legacy dependencies with current versions—with the
exception of bourbon, neat, and fontawesome—and drops others that are no
longer relevant (such as imageoptim).

### Why is it needed?
Bring 2.x more in line with 3.x, mainly allow use of modern js features.

### How to test

1. Run the rebuild processes, including `npm update` within the
`_build/templates/default` directory.
2. Run `grunt build`. 
3. Clear your manager and browser cache, then browse around the manager
with your console open to verify all works as expected and no errors are
being reported.

Note that grunt build will spit out some warnings, as the versions of
bourbon and neat we need to stick with here (for now at least) are
ancient and contain some long-deprecated code. I attempted to bring
these dependencies up to date (including fontawesome) but there are many
breaking changes that make it difficult to unwind and get everything
working. Might try that later if there's enough "life" left in the 2.x
line and it's deemed beneficial to do so.

### Related issue(s)/PR(s)
Resolves issues with building after including the following PRs: #16493
and #16467.

---------

Co-authored-by: Jason Coward <[email protected]>
@opengeek opengeek dismissed stale reviews from Mark-H and theboxer via 335ee75 March 22, 2024 14:19
@opengeek opengeek force-pushed the sebastian-marinescu-clearDirty-guard-clause branch from 242b4ec to 335ee75 Compare March 22, 2024 14:19
@opengeek opengeek changed the title Add guard-clause to clearDirty method [2.x] Add guard-clause to clearDirty method Mar 22, 2024
@opengeek opengeek merged commit 3446d08 into modxcms:2.x Mar 22, 2024
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
blocked Active participation around the pull request or issue required. Consensus is not reached. bug The issue in the code or project, which should be addressed.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants