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

SlugModifier ignores unsaved record data received through XHR #33

Closed
NamelessCoder opened this issue May 3, 2023 · 7 comments · Fixed by #37
Closed

SlugModifier ignores unsaved record data received through XHR #33

NamelessCoder opened this issue May 3, 2023 · 7 comments · Fixed by #37

Comments

@NamelessCoder
Copy link

When changing the page title and regenerating the page slug, the SlugModifier checks if the record was already saved (has uid) and if it does, it attempts to load the record from DB. SlugModifier receives $record which contains the record data in current unsaved state, but replaces this array entirely with data from the DB. The result is that any unsaved record value is completely ignored by SlugModifier.

Compare the behavior with and without EXT:masi:

  • Without, the TYPO3 core uses the record data submitted in XHR which may or may not match the persisted values. Slug is generated based on whatever value the editor sees in the form, saved or not, new record or existing record.
  • With, EXT:masi completely replaces the XHR-submitted record data with the DB-persisted record data if the record was previously saved causing any unsaved value to be ignored.

It's possible that simply changing this line:

https://github.com/b13/masi/blob/master/Classes/SlugModifier.php#L95

From: $this->recordData = $row;
To: $this->recordData = array_replace($row, $this->recordData);

...would yield the correct combination of every column from DB, with values from unsaved fields in XHR overriding already persisted values in DB.

@LinkPool2
Copy link

Yes, that's a real problem .

Our editors complain that the slug changes only on the second save after changing the page title.

fwg added a commit to fwg/masi that referenced this issue Dec 5, 2023
… pieces

This fixes b13#33 by applying  to the loaded record array
with the incoming record fields.

Also apply string casting to the slug pieces to avoid fatal error when
some other hook sets a record field as something else than string, e.g.
integer.
fwg added a commit to fwg/masi that referenced this issue Dec 5, 2023
This fixes b13#33 by applying array_replace to the loaded record array
with the incoming record fields.

Also apply string casting to the slug pieces to avoid fatal error when
some other hook sets a record field as something else than string, e.g.
integer.
@fwg
Copy link
Contributor

fwg commented Dec 5, 2023

Yep, absolutely crucial. Created a PR for this, as it blocks our v11 upgrade at the moment.

@bmack bmack closed this as completed in #37 Dec 5, 2023
@LinkPool2
Copy link

Hi,
I've just updated to masi 2.0.2 and rechecked this issue.
The issue is - in my opinion - the same as before.

SlugModifier.php
Line 95

is still:

$this->recordData = $row;

and not:

$this->recordData = array_replace($row, $record);

At least when I look here:

https://extensions.typo3.org/extension/download/masi/2.0.2/zip

When I check here, it's fine:

https://github.com/b13/masi/blob/master/Classes/SlugModifier.php

Mhhhh... I don't understand...

@NamelessCoder
Copy link
Author

Version 2.0.2 is from November and does NOT contain this patch. Version 2.0.3 is from December and DOES contain this patch. Your dependency is outdated; you will need to update to 2.0.3.

@LinkPool2
Copy link

ahhhh!

But there is no 2.0.3 in TYPO3 TER? right?

https://extensions.typo3.org/extension/masi

@NamelessCoder
Copy link
Author

But there is no 2.0.3 in TYPO3 TER? right?

No, doesn't look like there is. You'd have to ask the maintainers why - my guess is an expired TER token prevented the automatic TER upload. But I definitely recommend using composer and only composer ;)

@LinkPool2
Copy link

"But I definitely recommend using composer and only composer ;)"

Yes, but we can't change this on a running project... next update... :)

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 a pull request may close this issue.

3 participants