Skip to content

Commit

Permalink
Fix issue #582 with exterannous error message appearing in FieldtypeF…
Browse files Browse the repository at this point in the history 
…ile.
  • Loading branch information
@ryancramerdesign
ryancramerdesign committed Aug 7, 2014
1 parent 9264a53 commit c1dd1b3
Showing 1 changed file with 12 additions and 1 deletion.
13 changes: 12 additions & 1 deletion wire/modules/Fieldtype/FieldtypeFile.module
View file
Expand Up @@ -60,7 +60,18 @@ class FieldtypeFile extends FieldtypeMulti {

$this->setupHooks($page, $field, $inputfield);

if(!$field->extensions) $this->error(sprintf($this->_('Field "%s" is not yet ready to use and needs to be configured.'), $field->name));
if(!$field->extensions) {
if($page->id && $page->template->fieldgroup->hasField($field)) {
// message that appears on page being edited with this field
$this->error(sprintf($this->_('Field "%s" is not yet ready to use and needs to be configured.'), $field->name));
} else if(!count($_POST)) {
// message that appears during configuration, but suppressed during post to prevent it from appearing after save
$this->message(
$this->_('Settings have not yet been committed.') . "<br /><small>" .
$this->_('Please review the settings on this page and save once more (even if you do not change anything) to confirm you accept them.') . "</small>",
Notice::allowMarkup);
}
}

return $inputfield;
}
Expand Down

5 comments on commit c1dd1b3

@adrianbj
Copy link

Choose a reason for hiding this comment

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

Hey Ryan - I think this is definitely an improvement, but I am worried that as a message, rather than a warning, I don't think people will see it. If you don't see it and proceed to add it to a template, and then edit a page with that template, you end up with the good old " FieldtypeImage: Field "newimage" is not yet ready to use and needs to be configured." error. If you go back to the field settings that same error shows again, which comes back to the same problem - what do I need to do to configure it :) and, if I save the field again at this point, the error still shows - so it still needs two saves to dismiss it.

It's a shame the default config can't be saved automatically.

Perhaps one improvement would be for the first message to be a warning - would you consider adding a new "warning" type to PW that was amber/orange. I think this would still get people's attention that something needs to be done, but without being a real error in this case. I know I have thought that would be useful in some of my modules.

Thanks!

@ryancramerdesign
Copy link
Owner Author

Choose a reason for hiding this comment

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

We don't have warnings, just messages and errors (someday we'll have to add warnings). Messages are meant to be informational or help you along, and that's what this is. If the user goes and tries to put content in the field before they've completed the field configuration, then at that point it's an error. I would consider an un-configured files field unsafe to use, so that's why I'm not comfortable with just falling back to defaults when they don't configure it. The same goes for plenty of other Fieldtypes too, though the safety issue doesn't have the same gravity, so less of a concern elsewhere.

I couldn't reproduce the issue you mentioned of having to save the field two times after creating it. I could reproduce that one before today, but not after this fix. The reason it was showing a second time is just because the check was done before the settings were saved. Combined with a 2x page load (save + redirect), it would appear twice. That's what I fixed. If it's still still saying "review and save" (or the unconfigured error in the page editor) even after you have already seen that message and followed the instruction, then we probably need to look at what's different about your system. I've got quite a few updates in progress, so it's always possible I've forgotten to push some file to GitHub.

@adrianbj
Copy link

Choose a reason for hiding this comment

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

I know we don't have warnings - that's why I suggested it ;) - I think it would be very useful, especially in this case because I honestly don't think anyone will read the message and know they have to save it again. I know I often ignore anything in green!

Did you follow my exact steps? The double save is only an issue if you end up with the warning after adding to template and trying to edit a page. I don't ever see the "review and save" message twice, but the unconfigured error definitely shows twice and is still not very helpful as to what you need to do to fix it. I know it is obvious to us, but I think someone new needs to see the "review and save" again if they follow these same steps, having missed the message the first time. Make sense?

Or maybe it is something you haven't committed yet?

@owzim
Copy link

@owzim owzim commented on c1dd1b3 Oct 29, 2014

Choose a reason for hiding this comment

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

I am still seeing this warning. What is it? I am creating image fields via api, and saving twice via api does not make it disappear, but saving the field in admin does.

@adrianbj
Copy link

Choose a reason for hiding this comment

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

I haven't done this in a while, but previously the only way around this for me via the API has been:

$ft = $this->modules->get(FieldtypeFile); // or FieldtypeImage etc
$ft->getDatabaseSchema($field);

If you also enable tags, I found I had to:

$sql = "ALTER TABLE `field_fieldname` ADD `tags` TEXT NOT NULL";
$query = $this->wire('database')->prepare($sql);
$query->execute();

This is the combo I am using in Migrator and seems to work great.

Please sign in to comment.