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

copying font-awesome fonts might fails #1595

Closed
sijad opened this issue Oct 6, 2018 · 13 comments
Closed

copying font-awesome fonts might fails #1595

sijad opened this issue Oct 6, 2018 · 13 comments

Comments

@sijad
Copy link
Contributor

sijad commented Oct 6, 2018

Bug Report

Current Behavior
font-awesome fonts will copy to public/assets/fonts only once after installation:

https://github.com/flarum/core/blob/8474dfd6e2cc8298176f0e088f0786ef099d0b73/src/Install/Console/InstallCommand.php#L323-L329

copyDirectory does not throw any errors, if it fails install will continue.
there's no easy way to copy fonts after installations.

Expected Behavior
fonts files should be copied in other occasions like clearing the cache or after migrations.

Environment

  • Flarum version: dev-master (beta.8)

Possible Solution

  • check copyDirectory result and throw an error.
  • copy fonts after cache:clear command.
@franzliedke
Copy link
Contributor

@sijad Can you clarify what failure you are encountering? Write permissions?

@dsevillamartin
Copy link
Member

Theoretically, there should be no write permissions as the web installation does not let you install Flarum without write access to public/.
Did you encounter this error while using the flarum binary to install Flarum? If so, we may want to add directory checks (that can be bypassed with a flag) to the command itself.

@sijad
Copy link
Contributor Author

sijad commented Oct 7, 2018

i thought it was related to permissions, I'll check again. and no i was using web installer.
shouldn't this kind coping hanled with composer hook? user might manually upgrade their font-awesome package version.

@dsevillamartin
Copy link
Member

@sijad Users can update FA 5 manually, as components/font-awesome is stuck at 5.0.6 and the latest FA is 5.3.1... though that would be a different issue than here.
Maybe we want a command in the Flarum binary to re-copy assets from flarum/core & extensions, just in case something goes wrong.

@luceos
Copy link
Member

luceos commented Oct 8, 2018

Can't we use the newly introduced webpack functionality to take care of all this? FA is available in npm as well, we can configure webpack to deal with the font by publishing it to the assets directory. This will reduce the complexity in our composer file, ensure the fonts exist every time the compilation is being ran and offers a later version of FA.

@dsevillamartin
Copy link
Member

dsevillamartin commented Oct 8, 2018

@luceos Wouldn't that require the user to install the npm packages then, though?
We may want to include SVG in the JS file instead (obv not every font but the ones that are used), as user installations cannot copy the FA files from a node_modules folder that hasn't been generated because they need Node, which would be a pain in shared hosting.

EDIT: Nvm, I misread your second sentence. That could be done with file-loader in webpack. I assume it would be saved to the assets folder which Flarum then copies for extensions, not sure about flarum/core. That would require the file names to always be the same, however, or some way of compiling less with the proper font files path.
Example:

{
  test: /\.(|woff|woff2|eot|ttf)$/i,
  use: [
    {
      loader: 'file-loader',
      options: {
        name: '../assets/fonts/[name].[ext]',
        context: 'src',
      }
    }
  ]
}

@luceos
Copy link
Member

luceos commented Oct 9, 2018

@datitisev yes sorry I wanted to PR it, but didn't get it working in time. Here's the snippet I used in another project:

      {
        test: /\.(ttf|otf|eot|svg|woff(2)?)(\?[a-z0-9]+)?$/,
        exclude: path.resolve(__dirname, 'assets/'),
        use: [{
          loader: 'file-loader',
          options: {
            useRelativePath: false,
            outputPath: 'webfonts/',
            publicPath: '../webfonts/',
            name: '[name].[ext]'
          }
        }]
      },

@tobyzerner
Copy link
Contributor

Remember that webpack doesn't run on production installations, only on development installations.

@dsevillamartin
Copy link
Member

@tobscure The plan here would be to have the fonts in an assets/ folder in flarum/core, as in extensions. The question is, would Flarum copy that folder if it existed, or does it need to be implemented? It would definitely improve the asset handling as right now we can't i.e. use npm modules with css, but with the assets folder this would be made possible.

@tobyzerner
Copy link
Contributor

Ah I see. I don't believe there's any code that would copy as assets folder in flarum/core. That could be a good idea.

@franzliedke franzliedke added this to the 0.1.0-beta.10 milestone Oct 10, 2018
@clarkwinkelmann
Copy link
Member

If #1644 is implemented this would be the place to try copying the fonts again.

@Ralkage Ralkage modified the milestones: 0.1.0-beta.10, 0.1.0-beta.11 Sep 10, 2019
@luceos luceos modified the milestones: 0.1.0-beta.11, 0.2 Sep 27, 2019
@clarkwinkelmann
Copy link
Member

I believe this issue is actually solved since assets copy was moved to the migrate command.

#1644 can be used to track the possible change from migrate to something else.

I suggest we close this.

@askvortsov1
Copy link
Member

I'm fine with closing as well.

@askvortsov1 askvortsov1 removed this from the 0.2 milestone Jul 19, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

8 participants