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

API Remove code which is being migrated to a new TinyMCE module #11617

Merged
merged 1 commit into from
Mar 14, 2025

Conversation

GuySartorelli
Copy link
Member

@GuySartorelli GuySartorelli commented Feb 18, 2025

Important

Make sure you test this without the new tinymce module installed as well to ensure the textarea HTML editor field works, and that on save any disallowed elements/attributes are removed.
All other PRs will still need to be included because otherwise PHP will fail to run because there are references to missing classes lol

Issue

@GuySartorelli GuySartorelli marked this pull request as draft February 18, 2025 00:55
@GuySartorelli GuySartorelli force-pushed the pulls/6.0/tinymce branch 4 times, most recently from d3eb6c1 to 58567b9 Compare February 25, 2025 00:36
@GuySartorelli GuySartorelli force-pushed the pulls/6.0/tinymce branch 10 times, most recently from 27e8dfa to b94410c Compare March 5, 2025 00:03
@GuySartorelli GuySartorelli force-pushed the pulls/6.0/tinymce branch 6 times, most recently from 8036736 to 19ec36b Compare March 6, 2025 23:06
@GuySartorelli GuySartorelli force-pushed the pulls/6.0/tinymce branch 2 times, most recently from 5db8ee1 to f8f976e Compare March 10, 2025 04:55
@@ -3,20 +3,276 @@ Name: corehtml
---
Copy link
Member

Choose a reason for hiding this comment

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

Give this a name like framework-html so that it's easier for people to After: in their own yml

I had to After: '*' in my project to get my config to override values here

Copy link
Member Author

Choose a reason for hiding this comment

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

It already has a name on Line 2:

Name: corehtml

Copy link
Member Author

Choose a reason for hiding this comment

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

I'll add After: 'corehtml' in the docs though, it's good you called this out I didn't consider that might be necessary.

$this->validValues = (array) $value;
break;
default:
throw new InvalidArgumentException('$valueType must be one of the VALUE_* consts.');
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
throw new InvalidArgumentException('$valueType must be one of the VALUE_* consts.');
throw new InvalidArgumentException('$valueType must be one of the HTMLEditorAttributeRule::VALUE_* consts.');

Copy link
Member Author

Choose a reason for hiding this comment

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

Done

}

/**
* Undocumented function
Copy link
Member

Choose a reason for hiding this comment

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

Should document it

Copy link
Member Author

@GuySartorelli GuySartorelli Mar 11, 2025

Choose a reason for hiding this comment

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

Done but please include the method name when suggesting changes like these - all I have to go off is the line number to know what specific method this is for, and line numbers can change as I apply other recommended changes.

* Undocumented function
*
* @param string $regex
* @return string
Copy link
Member

Choose a reason for hiding this comment

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

Probably delete the docblock param / return type

Copy link
Member Author

Choose a reason for hiding this comment

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

Not sure what method this was but I've documented all methods with "Undocumented function"

}

/**
* Undocumented function
Copy link
Member

Choose a reason for hiding this comment

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

Should document

Copy link
Member Author

Choose a reason for hiding this comment

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

Done

@GuySartorelli GuySartorelli marked this pull request as ready for review March 13, 2025 00:37
@emteknetnz emteknetnz merged commit 4b0c758 into silverstripe:6.0 Mar 14, 2025
5 of 13 checks passed
@emteknetnz emteknetnz deleted the pulls/6.0/tinymce branch March 14, 2025 00:32
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.

2 participants