If you try to use the core locale module import form ( at admin/config/regional/translate/import ) when also having the JSONAPI and editor module enabled, you're presented with a LogicException, "LogicException: PO files URL should not be public." .

Full backtrace looks like:

file_create_url('translations://en_0.po') (Line: 27)
Drupal\jsonapi\Field\FileDownloadUrl->fileCreateRootRelativeUrl('translations://en_0.po') (Line: 84)
Drupal\jsonapi\Field\FileDownloadUrl->initList() (Line: 59)
Drupal\jsonapi\Field\FileDownloadUrl->getIterator() (Line: 559)
_editor_get_file_uuids_by_field(Object) (Line: 354)
editor_entity_insert(Object)
call_user_func_array('editor_entity_insert', Array) (Line: 402)
Drupal\Core\Extension\ModuleHandler->invokeAll('entity_insert', Array) (Line: 169)
Drupal\Core\Entity\EntityStorageBase->invokeHook('insert', Object) (Line: 441)
Drupal\Core\Entity\ContentEntityStorageBase->invokeHook('insert', Object) (Line: 470)
Drupal\Core\Entity\EntityStorageBase->doPostSave(Object, ) (Line: 326)
Drupal\Core\Entity\ContentEntityStorageBase->doPostSave(Object, ) (Line: 395)
Drupal\Core\Entity\EntityStorageBase->save(Object) (Line: 768)
Drupal\Core\Entity\Sql\SqlContentEntityStorage->save(Object) (Line: 364)
Drupal\Core\Entity\Entity->save() (Line: 905)
file_save_upload('file', Array, 'translations://', 0) (Line: 157)
Drupal\locale\Form\ImportForm->validateForm(Array, Object)
call_user_func_array(Array, Array) (Line: 83)
Drupal\Core\Form\FormValidator->executeValidateHandlers(Array, Object) (Line: 270)
Drupal\Core\Form\FormValidator->doValidateForm(Array, Object, 'locale_translate_import_form') (Line: 119)
Drupal\Core\Form\FormValidator->validateForm('locale_translate_import_form', Array, Object) (Line: 571)
Drupal\Core\Form\FormBuilder->processForm('locale_translate_import_form', Array, Object) (Line: 314)
Drupal\Core\Form\FormBuilder->buildForm('locale_translate_import_form', Object) (Line: 74)
Drupal\Core\Controller\FormController->getContentResult(Object, Object)
call_user_func_array(Array, Array) (Line: 123)
Drupal\Core\EventSubscriber\EarlyRenderingControllerWrapperSubscriber->Drupal\Core\EventSubscriber\{closure}() (Line: 574)
Drupal\Core\Render\Renderer->executeInRenderContext(Object, Object) (Line: 124)
Drupal\Core\EventSubscriber\EarlyRenderingControllerWrapperSubscriber->wrapControllerExecutionInRenderContext(Array, Array) (Line: 97)
Drupal\Core\EventSubscriber\EarlyRenderingControllerWrapperSubscriber->Drupal\Core\EventSubscriber\{closure}()
call_user_func_array(Object, Array) (Line: 144)
Symfony\Component\HttpKernel\HttpKernel->handleRaw(Object, 1) (Line: 64)
Symfony\Component\HttpKernel\HttpKernel->handle(Object, 1, 1) (Line: 57)
Drupal\Core\StackMiddleware\Session->handle(Object, 1, 1) (Line: 47)
Drupal\Core\StackMiddleware\KernelPreHandle->handle(Object, 1, 1) (Line: 38)
Drupal\jsonapi\StackMiddleware\FormatSetter->handle(Object, 1, 1) (Line: 47)
Drupal\Core\StackMiddleware\ReverseProxyMiddleware->handle(Object, 1, 1) (Line: 50)
Drupal\Core\StackMiddleware\NegotiationMiddleware->handle(Object, 1, 1) (Line: 23)
Stack\StackedHttpKernel->handle(Object, 1, 1) (Line: 656)
Drupal\Core\DrupalKernel->handle(Object) (Line: 19)

The root cause is that JSON API FileDownloadUrl assumes that all file URIs can be passed down to file_create_url, but core TranslationStream explicitely made its getExternalUrl call throw an exception, because those files should never be accessed publicly.

Not sure yet how to best fix that ; the exception should probably be catched, but in FileDownloadUrl::fileCreateRootRelativeUrl, but there's no correct value to return… If fixed, the related core issue #2449895: Imported PO files have no public URL but have a link at admin/content/files would make file_create_url return FALSE for this case.

CommentFileSizeAuthor
#6 2915810-6.patch1.21 KBwim leers
#2 2915810-2-catch-logicexception-translations.patch723 bytesDeFr

Comments

DeFr created an issue. See original summary.

DeFr’s picture

DeFr commented
Status: Active » Needs review
StatusFileSize
new 2915810-2-catch-logicexception-translations.patch723 bytes

Attaching a quick patch that gets the import working, and stops exception showing up in the jsonapi/file/file endpoint when a translations:// file gets shown up there.

That being said, as a webservice consumer, getting information about a file but having no url to fetch it is probably not that useful.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Status: Needs review » Needs work

Thanks for reporting this, and for the great initial patch! 👏

class TranslationsStream extends LocalStream {

  /**
   * {@inheritdoc}
   */
  public static function getType() {
    return StreamWrapperInterface::LOCAL_HIDDEN;
  }

Checking the type of a stream is the proper way of handling this.

DeFr’s picture

DeFr commented

@Wim Leers: This is part of the reason I've linked the core issue ;-) If you check the patch up there ( https://www.drupal.org/files/issues/2449895-20.patch ), you'll notice that it makes file_create_url() respect that flag, but that it also needs to update TemporaryStream to make it stop using LOCAL_HIDDEN . If that patch goes is, no change is required in JSON API. If that patch doesn't go in, checking for VISIBLE in JSON API means that temporary file URLs will disappear from the JSON API responses, which will also be a bug in itself.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Title: Core interface translation import form broken » JSON API's added 'url' computed base field to File entities breaks core's interface translation import form
Issue tags: +API-First Initiative

I see.

So the reason the translation import is broken is:

  1. File entity doesn't have a way to get the URL, so JSON API adds a base field
  2. the import form uses file_save_upload(), which creates a File entity
  3. upon saving the File entity, the insert hooks are called
  4. one of the insert hooks is editor_entity_insert(), which iterates over all fields
  5. because it iterates over all fields, it also iterates over the base field the JSON API module is adding
  6. and in doing so, it calls \Drupal\locale\StreamWrapper\TranslationsStream::getExternalUrl(), which throws that exception

That is … very interesting and convoluted :)

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Status: Needs work » Needs review
StatusFileSize
new 2915810-6.patch1.21 KB

I'm fine with committing #2, but it'll only work for translations:// files. I think it's better to solve the problem more broadly, for all "special" stream wrappers. Patch attached. Looking forward to your feedback!

  • e0ipso committed 2c808b8 on 8.x-1.x authored by Wim Leers 
    fix(Translation): Download URL field causes transtation import issues (#...
e0ipso’s picture

e0ipso
Primary language Catalan
Location Can Picafort
commented
Status: Needs review » Fixed

Thanks all!

DeFr’s picture

DeFr commented

Sorry for taking time to respond. Not sure I was completly clear in #4, so I'm going to try to rephrase what I meant up there: with the patch that was committed, JSON API will no longer return the URL of temporary files, because TemporaryStorage::getType() returns LOCAL_HIDDEN , even though there's an actual URL for those files set at TemporaryStorage::getExternalUrl().

I feel like there might thus be some legitimate usage of JSON API that the commited patch broke.

e0ipso’s picture

e0ipso
Primary language Catalan
Location Can Picafort
commented

@DeFr I see what you mean. My call was that temporary files should not be exposed, only persisted information should.

However I'd like to be proven wrong. Please open an issue to add support to the download URL for temporary files and we'll ask for consensus/participation there.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented

My call was that temporary files should not be exposed, only persisted information should.

+1

However I'd like to be proven wrong. Please open an issue to add support to the download URL for temporary files and we'll ask for consensus/participation there.

+1 :)

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.