Skip to content

[4.0] Check if editor save function exists#17942

Closed
laoneo wants to merge 10 commits intojoomla:4.0-devfrom
Digital-Peak:j4/fix/editr/save
Closed

[4.0] Check if editor save function exists#17942
laoneo wants to merge 10 commits intojoomla:4.0-devfrom
Digital-Peak:j4/fix/editr/save

Conversation

@laoneo
Copy link
Copy Markdown
Member

@laoneo laoneo commented Sep 12, 2017

Summary of Changes

I can't save an article on the front end on the current staging branch because of the error:
Call to undefined method Joomla\CMS\Editor\Editor::save()

Testing Instructions

Edit an article on the front end.

Expected result

Article edit form opens and article can be edited.

Actual result

Error is displayed and article can't be edited.

@mbabker
Copy link
Copy Markdown
Contributor

mbabker commented Sep 12, 2017

Ping @DGT41 as this involves code removed from the editor and plugins...

same goes for all the other methods except display and init and getButtons
@dgrammatiko
Copy link
Copy Markdown
Contributor

@laoneo check Digital-Peak#5

There is no save() anymore
@laoneo
Copy link
Copy Markdown
Member Author

laoneo commented Sep 12, 2017

@DGT41 I think now we have a BC break here which we probably can avoid with my first suggestion. This code here https://github.com/joomla/joomla-cms/blob/4.0-dev/components/com_content/tmpl/form/edit.php#L35 breaks now in every extension.

@laoneo
Copy link
Copy Markdown
Member Author

laoneo commented Sep 12, 2017

I'v reverted the save function and marked it as deprecated, which I think is better than having a BC break here.

@dgrammatiko
Copy link
Copy Markdown
Contributor

dgrammatiko commented Sep 12, 2017

@laoneo the methods are deprecated since 3.7 and this is a needed B/C break.
The reason is that dropping the support for these old stuff we also enable multi-editors per page.
This is on purpose

EDIT: we can still provide empty function as we're doing in JHtml::behavior, so apps won't crash and burn (though I'm not in favour of doing that)

@laoneo
Copy link
Copy Markdown
Member Author

laoneo commented Sep 12, 2017

The function in the editors itself but not on the form field.

@dgrammatiko
Copy link
Copy Markdown
Contributor

Let's deprecate it in 3.8/3.9 and remove it in j4. Let's drag less unneeded baggages...

@dgrammatiko
Copy link
Copy Markdown
Contributor

If you want keep this can you always return null?

@laoneo
Copy link
Copy Markdown
Member Author

laoneo commented Sep 12, 2017

Removed the function as suggested and deprecated the form field function in staging trough pr #17944. Hopefully it will get into 3.8 to have enough time for extension devs to adapt.

{
if (task == 'article.cancel' || document.formvalidator.isValid(document.getElementById('adminForm')))
{
" . $this->form->getField('articletext')->save() . "
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

this is irrelevant to this PR, but since the form already has the class form-validate all this inline script is useless, the core.js covers this case internally

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I'm a bit hesitant to do it in this pr.

@wilsonge
Copy link
Copy Markdown
Contributor

wilsonge commented Feb 2, 2018

Is this still needed? I know @DGT41 's did a refactor on tiny at some point towards the end of last year and I've slightly lost track

@dgrammatiko
Copy link
Copy Markdown
Contributor

@wilsonge we already deprecated these methods in 3.x but we still need a pr to actually drop them in 4.0. Will do that later on today

@laoneo
Copy link
Copy Markdown
Member Author

laoneo commented Feb 3, 2018

That's what this pr is doing and the deprecation message is added in the pr #17944. Keep in mind, we talk here about the form field and not the editor itself.

@laoneo laoneo closed this Jun 11, 2018
@laoneo laoneo deleted the j4/fix/editr/save branch June 11, 2018 09:12
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.

5 participants